zhangqd / chromiumembedded

Automatically exported from code.google.com/p/chromiumembedded
0 stars 1 forks source link

Support HTTP charset and compression with CefResourceRequestJob #1070

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
CefResourceRequestJob tries to mimic URLRequestHttpJob yet does not handle 
charset and compression.

Attached patch provides support for gzip/deflate compression using the same 
mechanism as URLRequestHttpJob, i.e. Filters and finding out from the headers 
which encoding was used. SDCH is not implemented for simplicity (and also 
because it would require that users specified that sdch was advertised in 
CefResponse).

The patch also provides support for Charset, following the support for the MIME 
type.

Original issue reported on code.google.com by pgu...@gmail.com on 5 Sep 2013 at 9:51

Attachments:

GoogleCodeExporter commented 9 years ago
Thanks for the patch. Some comments:

1. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=1070&aid=107
00000000&name=cef3_resource_compression.diff&token=Lka1fmmvX9so9kKQdzlnQr5dnhY%3
A1378395124430#256

+CefResourceRequestJob::CefResourceFilterContext::CefResourceFilterContext(CefRe
sourceRequestJob* job)
+    : job_(job) {
+  DCHECK(job_);
+}

You can implement these methods inline with the class declaration. Also, wrap 
lines to 80 characters.

2. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=1070&aid=107
00000000&name=cef3_resource_compression.diff&token=Lka1fmmvX9so9kKQdzlnQr5dnhY%3
A1378395124430#462

+ private:
+   net::HttpResponseHeaders* DoGetResponseHeaders();

Too much indentation.

3. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=1070&aid=107
00000000&name=cef3_resource_compression.diff&token=Lka1fmmvX9so9kKQdzlnQr5dnhY%3
A1378395124430#442

-namespace net {
-class HttpResponseHeaders;
-}
+#include "net/http/http_response_headers.h"

All includes should be first and sorted alphabetically.

4. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=1070&aid=107
00000000&name=cef3_resource_compression.diff&token=Lka1fmmvX9so9kKQdzlnQr5dnhY%3
A1378395124430#372

+  CefResponseImpl* responseImpl =
         static_cast<CefResponseImpl*>(response_.get());

Too much indent.

5. Please add unit tests for the new functionality.

Original comment by magreenb...@gmail.com on 5 Sep 2013 at 3:41

GoogleCodeExporter commented 9 years ago
#1 About inlining, I tried to keep code as much like URLRequestHttpJob. Indeed, 
Chromium coding style seems to be a little bit less inclined towards inlining.

#4 I am confused about this one.

Original comment by pgu...@gmail.com on 5 Sep 2013 at 3:50

GoogleCodeExporter commented 9 years ago
> #1 About inlining, I tried to keep code as much like URLRequestHttpJob. 
Indeed, 
> Chromium coding style seems to be a little bit less inclined towards inlining.

You're correct that the style guide is mostly silent on this topic. However, 
consistency with existing code in the same file is important.

> #4 I am confused about this one.

Indent the 2nd line 4 spaces instead of 6.

Original comment by magreenb...@gmail.com on 5 Sep 2013 at 4:07

GoogleCodeExporter commented 9 years ago
Sorry to bother, do you will accept this patch? Being unable to set the charset 
for custom resorce handlers is quite silly ;)

Actually i use a custom cef3 build with this patch but my poor computer needs 1 
day to build debug and release buils of cef3.

Thanks!

Original comment by d.alb...@gmail.com on 11 Aug 2014 at 12:29

GoogleCodeExporter commented 9 years ago
CEF is transitioning from Google Code to Bitbucket project hosting. If you 
would like to continue receiving notifications on this issue please add 
yourself as a Watcher at the new location: 
https://bitbucket.org/chromiumembedded/cef/issue/1070

Original comment by magreenb...@gmail.com on 14 Mar 2015 at 3:27