Open GoogleCodeExporter opened 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
#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
> #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
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
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
Original issue reported on code.google.com by
pgu...@gmail.com
on 5 Sep 2013 at 9:51Attachments: