Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
981633002:
Created new URLRequestContext for secure proxy check (Closed)
DescriptionCreated new URLRequestContext for secure proxy check.
The new URLRequestContext disables alternate protocols
to ensure that the Data Reduction Proxy secure proxy check
request goes out over vanilla HTTP so that it can be
intercepted by middleboxes.
Also, secure proxy check is now done on IO thread.
BUG=437080
Committed: https://crrev.com/652eabf1141c00594aa6e9ed81beb980aec89198
Cr-Commit-Position: refs/heads/master@{#325302}
Committed: https://crrev.com/afc53166cb6a0ed561f416403ab68261eae4427b
Cr-Commit-Position: refs/heads/master@{#325464}
Committed: https://crrev.com/e9ceffc7d7758d47a45034b0b308b39eb7e515dc
Cr-Commit-Position: refs/heads/master@{#325674}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #
Total comments: 1
Patch Set 3 : Using URLRequestContextBuilder for secure proxy check #
Total comments: 10
Patch Set 4 : Addressed comments. #Patch Set 5 : Fixed the tests #
Total comments: 26
Patch Set 6 : Moved secure proxy check to DRPConfig #
Total comments: 24
Patch Set 7 : Moved Secure Proxy check to separate class. Addressed comments. #
Total comments: 63
Patch Set 8 : Addressed comments #
Total comments: 12
Patch Set 9 : Addressed comments #
Total comments: 8
Patch Set 10 : Addressed comments #
Total comments: 4
Patch Set 11 : Fixed formatting #Patch Set 12 : Rebased #Patch Set 13 : Fix missing header file #Patch Set 14 : Update data reduction proxy init in android webview #Patch Set 15 : Rebased again #Patch Set 16 : Fixed minor bug #
Total comments: 4
Patch Set 17 : Rebased on jeremyim@ CL which simplifies everything #
Total comments: 2
Patch Set 18 : Removed duplicate forward declarations #
Total comments: 14
Patch Set 19 : Addressed comments. #
Total comments: 3
Patch Set 20 : Addressed comments, submitting now. #Patch Set 21 : Delayed calling GetURLRequestContext. This was causing webview tests to fail. #Messages
Total messages: 114 (39 generated)
tbansal@chromium.org changed reviewers: + rch@chromium.org, sclittle@chromium.org
sclittle@chromium.org: Please review changes in * rch@chromium.org: Please review changes in net/*
Please update the subject line of this CL, it still says DISABLE_ALTERNATE_PROTOCOLS. Also, the probe is now called the "secure proxy check". https://codereview.chromium.org/981633002/diff/1/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/981633002/diff/1/net/base/load_flags_list.h#n... net/base/load_flags_list.h:105: // Indicates that the request should only use HTTP 1s. Did you mean "HTTP/1.1"? https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:751: using_ssl_ = (request_info_.load_flags & LOAD_UNENCRYPTED_HTTP11) == 0 && What happens if the load flag is set for a https URL? https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:760: http_server_properties->SetHTTP11Required(origin_); Won't this also force all future requests to this origin in the session to only use HTTP/1.1? https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:804: if (spdy_session && CanUseExistingSpdySession()) { What if there's an existing SPDY session? Could it still be possible for the LOAD_UNENCRYPTED_HTTP11 request to use the existing SPDY session?
Didn't we have https://codereview.chromium.org/920993002/ to do this? How come we changed CL numbers? (I'm always sad to lose context)
The semantics of the previous incarnation seem cleaner to me. Since your request will always be an http:// url, it seems like disabling alternate protocols gets you exactly what you want without needing to wade into some of the issues with this CL. https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:753: request_info_.url.SchemeIs("wss") || ShouldForceSpdySSL()); This definitely seems wrong. If HTTP11 is required for a secure origins (https or wss) that should simply be an error.
On 2015/03/05 01:43:57, Ryan Hamilton wrote: > The semantics of the previous incarnation seem cleaner to me. Since your request > will always be an http:// url, it seems like disabling alternate protocols gets > you exactly what you want without needing to wade into some of the issues with > this CL. > > https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... > net/http/http_stream_factory_impl_job.cc:753: request_info_.url.SchemeIs("wss") > || ShouldForceSpdySSL()); > This definitely seems wrong. If HTTP11 is required for a secure origins (https > or wss) that should simply be an error. Would disabling alternate protocols be enough? I was under the impression that if force_spdy_always is true, then it will use spdy. Also, there was this issue of transaction using HTTP 2.0 if the server is advertising it (which we do not want).
On 2015/03/05 02:12:13, tbansal1 wrote: > On 2015/03/05 01:43:57, Ryan Hamilton wrote: > > The semantics of the previous incarnation seem cleaner to me. Since your > request > > will always be an http:// url, it seems like disabling alternate protocols > gets > > you exactly what you want without needing to wade into some of the issues with > > this CL. > > > > > https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... > > File net/http/http_stream_factory_impl_job.cc (right): > > > > > https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... > > net/http/http_stream_factory_impl_job.cc:753: > request_info_.url.SchemeIs("wss") > > || ShouldForceSpdySSL()); > > This definitely seems wrong. If HTTP11 is required for a secure origins (https > > or wss) that should simply be an error. > > Would disabling alternate protocols be enough? I was under the impression > that if force_spdy_always is true, then it will use spdy. You mean if someone is running with a command line flag? I'm pretty sure that flag simply doesn't work and needs to be removed. We should nuke it :> > Also, > there was this issue of transaction using HTTP 2.0 if the server > is advertising it (which we do not want). If the URL is https:// then we'll use NPN/ALPN negotiation to use SPDY or HTTP/2. But your URL is not https:// so that's not an issue for you. The only mechanism for a server answering an http:// URL to tell chrome to do something other than vanilla-HTTP/1 is for it to use the Alternate-Protocol or Alternate-Service header. But the earlier CL disables that mechanism. So as long as your URL is http:// the old CL does what you want. The only caveat with that CL was the description of the flag suggesting that it disabled SPDY.
On 2015/03/05 20:13:35, Ryan Hamilton wrote: > On 2015/03/05 02:12:13, tbansal1 wrote: > > On 2015/03/05 01:43:57, Ryan Hamilton wrote: > > > The semantics of the previous incarnation seem cleaner to me. Since your > > request > > > will always be an http:// url, it seems like disabling alternate protocols > > gets > > > you exactly what you want without needing to wade into some of the issues > with > > > this CL. > > > > > > > > > https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... > > > File net/http/http_stream_factory_impl_job.cc (right): > > > > > > > > > https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... > > > net/http/http_stream_factory_impl_job.cc:753: > > request_info_.url.SchemeIs("wss") > > > || ShouldForceSpdySSL()); > > > This definitely seems wrong. If HTTP11 is required for a secure origins > (https > > > or wss) that should simply be an error. > > > > Would disabling alternate protocols be enough? I was under the impression > > that if force_spdy_always is true, then it will use spdy. > > You mean if someone is running with a command line flag? I'm pretty sure that > flag simply doesn't work and needs to be removed. We should nuke it :> > > > Also, > > there was this issue of transaction using HTTP 2.0 if the server > > is advertising it (which we do not want). > > If the URL is https:// then we'll use NPN/ALPN negotiation to use SPDY or > HTTP/2. But your URL is not https:// so that's not an issue for you. > > The only mechanism for a server answering an http:// URL to tell chrome to do > something other than vanilla-HTTP/1 is for it to use the Alternate-Protocol or > Alternate-Service header. But the earlier CL disables that mechanism. > > So as long as your URL is http:// the old CL does what you want. The only caveat > with that CL was the description of the flag suggesting that it disabled SPDY. Yes, the URL is http://* If using http:// is enough, that makes things easier.
Addressed comments. Not explicitly disabling spdy anymore when the UNENCRYPTED_HTTP11 flag is up. https://codereview.chromium.org/981633002/diff/1/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/981633002/diff/1/net/base/load_flags_list.h#n... net/base/load_flags_list.h:105: // Indicates that the request should only use HTTP 1s. On 2015/03/04 21:35:23, sclittle wrote: > Did you mean "HTTP/1.1"? Done. https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:751: using_ssl_ = (request_info_.load_flags & LOAD_UNENCRYPTED_HTTP11) == 0 && On 2015/03/04 21:35:23, sclittle wrote: > What happens if the load flag is set for a https URL? Done. https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:753: request_info_.url.SchemeIs("wss") || ShouldForceSpdySSL()); On 2015/03/05 01:43:57, Ryan Hamilton wrote: > This definitely seems wrong. If HTTP11 is required for a secure origins (https > or wss) that should simply be an error. Done. https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:760: http_server_properties->SetHTTP11Required(origin_); On 2015/03/04 21:35:23, sclittle wrote: > Won't this also force all future requests to this origin in the session to only > use HTTP/1.1? Done. https://codereview.chromium.org/981633002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl_job.cc:804: if (spdy_session && CanUseExistingSpdySession()) { On 2015/03/04 21:35:23, sclittle wrote: > What if there's an existing SPDY session? Could it still be possible for the > LOAD_UNENCRYPTED_HTTP11 request to use the existing SPDY session? Done.
I don't think I understand how these changes prevent SPDY (or SSL for that matter) from being used. The DCHECKS, of course, are compiled out in a release build. If you don't care about preventing SSL from being used (which I think is fine because your URL is http://) I really think the other CL is a better choice. https://codereview.chromium.org/981633002/diff/20001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/981633002/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:723: (request_info_.load_flags & LOAD_UNENCRYPTED_HTTP11) == 0; This method is also only called from a command line argument. I'm not sure how important it is for the load flag to work in this case.
sclittle@chromium.org changed reviewers: + bengr@chromium.org
On 2015/03/06 23:19:53, Ryan Hamilton wrote: > I don't think I understand how these changes prevent SPDY (or SSL for that > matter) from being used. The DCHECKS, of course, are compiled out in a release > build. > > If you don't care about preventing SSL from being used (which I think is fine > because your URL is http://) I really think the other CL is a better choice. > > https://codereview.chromium.org/981633002/diff/20001/net/http/http_stream_fac... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/981633002/diff/20001/net/http/http_stream_fac... > net/http/http_stream_factory_impl_job.cc:723: (request_info_.load_flags & > LOAD_UNENCRYPTED_HTTP11) == 0; > This method is also only called from a command line argument. I'm not sure how > important it is for the load flag to work in this case. Ryan, I was trying to reason about this CL and ran into a more fundamental question. In the current world, afaik the only way to use quic is by negotiating for an alternate protocol. So currently it makes sense to have an http:// url use quic. The request starts off without TLS, but after negotiation it would get TLS. Now, let's say quic becomes the standard transport for HTTP. Then would it make sense ever to have http:// urls anymore, or should all urls be https:// because TLS comes with quic? Would http:// mean 'do not use quic'?
On 2015/03/09 17:42:35, bengr wrote: > I was trying to reason about this CL and ran into a more fundamental question. > In the current world, afaik the only way to use quic is by negotiating for an > alternate protocol. So currently it makes sense to have an http:// url use quic. > The request starts off without TLS, but after negotiation it would get TLS. I'm not sure what you mean by TLS here. With QUIC, we have "secure QUIC" and "insecure QUIC" both version encrypt the packets, but secure QUIC uses server certificates and insecure QUIC does not. THis means that insecure QUIC is trivially man-in-the-middle-able. But neither use TLS per-se. > Now, > let's say quic becomes the standard transport for HTTP. Then would it make sense > ever to have http:// urls anymore, or should all urls be https:// because TLS > comes with quic? Would http:// mean 'do not use quic'? Everyone should only use https:// URLs because security is good :> That being said, if QUIC becomes a standard it will still be announced (as far as we've ever talked about) via the Alternate-Protocol header (well, really Alt-Svc which is the new IETF standard for basically the same thing). "https://" means "use server certificates. "http://" means "don't use server certificates". QUIC can be spoken for both URL types. (But if "DISABLE_ALTERNATE_PROTOCOLS" is set, I we wouldn't use QUIC)
ptal.
https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:70: builder.set_proxy_config_service( If in the future someone adds a new member to URLRequestContext, this might have to be updated. Maybe you could use URLRequestContext::CopyFrom somehow? Also, this method will run on the UI thread, but the URLRequestContext would be used on the IO thread. Is that OK? https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:85: net::HttpNetworkSession::Params* params_original = If you use URLRequestContext::CopyFrom, I think you get the Params for free. Is it possible to avoid casting a |const net::HttpNetworkSession::Params*| to |net::HttpNetworkSession::Params*| here? https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:14: #include "net/url_request/url_request_context_getter.h" Is this include needed? https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:93: // URLRequestContextGetter to be used for the secure proxy check. It's confusing that there's both a |url_request_context_getter_| member and a |getter_secure_proxy_check_| member. Maybe remove one? Also, shouldn't this be a scoped_refptr? https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:208: *(const_cast<const net::HttpNetworkSession::Params*>(params))); Is this const_cast necessary?
thanks for the comments. ptal. https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:70: builder.set_proxy_config_service( On 2015/03/13 01:58:26, sclittle wrote: > If in the future someone adds a new member to URLRequestContext, this might have > to be updated. Maybe you could use URLRequestContext::CopyFrom somehow? > > Also, this method will run on the UI thread, but the URLRequestContext would be > used on the IO thread. Is that OK? Using CopyFrom() now Also, switched to IO thread https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:85: net::HttpNetworkSession::Params* params_original = On 2015/03/13 01:58:26, sclittle wrote: > If you use URLRequestContext::CopyFrom, I think you get the Params for free. Is > it possible to avoid casting a |const net::HttpNetworkSession::Params*| to > |net::HttpNetworkSession::Params*| here? Using CopyFrom() now. Not sure how to avoid const_cast. This (https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...) returns a const and if I need to modify enable_quic, at some point I would need to do const_cast...right? https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:14: #include "net/url_request/url_request_context_getter.h" On 2015/03/13 01:58:27, sclittle wrote: > Is this include needed? Moved it to .cc where it is needed. https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:93: // URLRequestContextGetter to be used for the secure proxy check. On 2015/03/13 01:58:26, sclittle wrote: > It's confusing that there's both a |url_request_context_getter_| member and a > |getter_secure_proxy_check_| member. Maybe remove one? > > Also, shouldn't this be a scoped_refptr? Removed one of them. https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:208: *(const_cast<const net::HttpNetworkSession::Params*>(params))); On 2015/03/13 01:58:27, sclittle wrote: > Is this const_cast necessary? Done.
https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:375: base::Unretained(this)), io_task_runner_); Why not just send the secure proxy check from DRPConfig? https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:70: net::URLRequestContext* url_request_context( Who owns this memory? I don't see anyone deleting it. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:80: net::HttpNetworkSession::Params params_modified = *params_original; Why do you need to const_cast? Why can't you just dereference the return from params_original and assign to params_modified? https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:115: FetcherResponseCallback fetcher_callback, const& ? https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:89: void SecureProxyCheckOnIOThread(const GURL& secure_proxy_check_url, Please add a comment. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:197: // Enabling QUIC should have no effect since secure proxy should not I don't understand. Why are you enabling QUIC if it will have no effect? What exactly are you testing? https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:207: //context.set_http_network_session_params( Remove dead code. https://codereview.chromium.org/981633002/diff/80001/net/url_request/url_requ... File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/981633002/diff/80001/net/url_request/url_requ... net/url_request/url_request_test_util.h:75: const_cast<HttpNetworkSession::Params*>(¶ms)); Shouldn't you just change set_http_network_session_params to take a pointer?
https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:15: #include "net/url_request/url_request_context_builder.h" Some of these includes aren't used. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:83: params_modified.use_alternate_protocols = false; I think you just need to clear params_modified.next_protos and set enable_quic = false, see: https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:89: new net::HttpNetworkLayer(network_session.get())); I don't think the URLRequestContext actually takes ownership of the HttpTransactionFactory; you'll have to manage the ownership yourself. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:92: scoped_refptr<net::URLRequestContextGetter> getter_secure_proxy_check( Ideally, this URLRequestContextGetter would own the URLRequestContext somehow, so that when the URLFetcher is destroyed, the URLRequestContext is also destroyed. Since all this stuff lives on the IO thread, maybe you could create your own SecureProxyCheckURLRequestContextGetter or something that owns the URLRequestContext; you could probably give it ownership of the HttpTransactionFactory as well.
ptal https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:375: base::Unretained(this)), io_task_runner_); On 2015/03/16 23:21:06, bengr wrote: > Why not just send the secure proxy check from DRPConfig? Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:15: #include "net/url_request/url_request_context_builder.h" On 2015/03/18 22:52:15, sclittle wrote: > Some of these includes aren't used. Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:70: net::URLRequestContext* url_request_context( On 2015/03/16 23:21:06, bengr wrote: > Who owns this memory? I don't see anyone deleting it. Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:80: net::HttpNetworkSession::Params params_modified = *params_original; On 2015/03/16 23:21:06, bengr wrote: > Why do you need to const_cast? Why can't you just dereference the return from > params_original and assign to params_modified? Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:83: params_modified.use_alternate_protocols = false; On 2015/03/18 22:52:15, sclittle wrote: > I think you just need to clear params_modified.next_protos and set enable_quic = > false, see: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:89: new net::HttpNetworkLayer(network_session.get())); On 2015/03/18 22:52:15, sclittle wrote: > I don't think the URLRequestContext actually takes ownership of the > HttpTransactionFactory; you'll have to manage the ownership yourself. Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:89: new net::HttpNetworkLayer(network_session.get())); On 2015/03/18 22:52:15, sclittle wrote: > I don't think the URLRequestContext actually takes ownership of the > HttpTransactionFactory; you'll have to manage the ownership yourself. Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:92: scoped_refptr<net::URLRequestContextGetter> getter_secure_proxy_check( On 2015/03/18 22:52:15, sclittle wrote: > Ideally, this URLRequestContextGetter would own the URLRequestContext somehow, > so that when the URLFetcher is destroyed, the URLRequestContext is also > destroyed. > > Since all this stuff lives on the IO thread, maybe you could create your own > SecureProxyCheckURLRequestContextGetter or something that owns the > URLRequestContext; you could probably give it ownership of the > HttpTransactionFactory as well. Managing the memory in a similar way as it was done for fetcher_. Creating new class might be overkill? https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:115: FetcherResponseCallback fetcher_callback, On 2015/03/16 23:21:06, bengr wrote: > const& ? Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:115: FetcherResponseCallback fetcher_callback, On 2015/03/16 23:21:06, bengr wrote: > const& ? Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:89: void SecureProxyCheckOnIOThread(const GURL& secure_proxy_check_url, On 2015/03/16 23:21:06, bengr wrote: > Please add a comment. Done. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:197: // Enabling QUIC should have no effect since secure proxy should not On 2015/03/16 23:21:06, bengr wrote: > I don't understand. Why are you enabling QUIC if it will have no effect? What > exactly are you testing? Added more details to the comment. https://codereview.chromium.org/981633002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:207: //context.set_http_network_session_params( On 2015/03/16 23:21:06, bengr wrote: > Remove dead code. Done. https://codereview.chromium.org/981633002/diff/80001/net/url_request/url_requ... File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/981633002/diff/80001/net/url_request/url_requ... net/url_request/url_request_test_util.h:75: const_cast<HttpNetworkSession::Params*>(¶ms)); On 2015/03/16 23:21:06, bengr wrote: > Shouldn't you just change set_http_network_session_params to take a pointer? Done.
https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:431: url_request_context_.reset(new net::URLRequestContext()); This could possibly destroy the URLRequestContext for a secure proxy check that's in progress if the user toggles Data Saver on and off repeatedly really fast. The URLRequestContext would also be destroyed prematurely if the DRPConfig is destroyed before the URLFetcher gets destroyed. I'd suggest that you create your own SecureProxyCheckURLRequestContextGetter class in this file that takes ownership of the URLRequestContext and the HttpNetworkLayer, and lazily initialize that URLRequestContextGetter, having DRPConfig hold on to a scoped_refptr of it, and reuse it for later secure proxy checks. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:433: url_request_context_getter_->GetURLRequestContext()); I think it's only safe to use the profile's URLRequestContextGetter GetURLRequestContext() on the UI thread, see: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:435: const net::HttpNetworkSession::Params* params_original = This variable is unnecessary. You could just have: DCHECK(url_request_context->GetNetworkSessionParams()); net::HttpNetworkSession::Params params = *url_request_context->GetNetworkSessionParams(); or something like that. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:446: http_network_layer.reset(new net::HttpNetworkLayer(network_session.get())); You could get rid of the network_session variable and just have "new net::HttpNetworkLayer(new net::HttpNetworkSession(params))" https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:452: getter_secure_proxy_check_ = new net::TrivialURLRequestContextGetter( You're constructing a new URLRequestContext and URLRequestContextGetter for every secure proxy check; why not just lazily construct it once and re-use it? https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:231: scoped_ptr<net::HttpNetworkLayer> http_network_layer; needs a "_" at the end. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:15: #include "net/url_request/url_request_context_getter.h" Unnecessary include. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:43: net::URLRequestContextGetter* request_context_getter, I think the net::URLRequestContextGetter from the BrowserContext is only allowed to be used on the UI thread. See: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... What you probably have to do is call GetURLRequestContext on the UI thread, then pass the URLRequestContext to the DRPConfig on the IO thread.
https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:248: void DataReductionProxyConfig::HandleSecureProxyCheckResponse( It would probably be better to create a new class called SecureProxyChecker and have DRPConfig own it. It could have a method CheckIfSecureProxyIsAllowed, that takes a callback. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:418: DCHECK(source == fetcher_.get()); DCHECK_EQ? https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:429: DCHECK(url_request_context_getter_); Can we DCHECK this on construction instead? https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:96: new net::TestURLRequestContext(true)); Indentation.
What's happening with this CL?
ptal https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:248: void DataReductionProxyConfig::HandleSecureProxyCheckResponse( On 2015/03/24 15:39:12, bengr wrote: > It would probably be better to create a new class called SecureProxyChecker and > have DRPConfig own it. It could have a method CheckIfSecureProxyIsAllowed, that > takes a callback. Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:418: DCHECK(source == fetcher_.get()); On 2015/03/24 15:39:12, bengr wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:429: DCHECK(url_request_context_getter_); On 2015/03/24 15:39:12, bengr wrote: > Can we DCHECK this on construction instead? Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:431: url_request_context_.reset(new net::URLRequestContext()); On 2015/03/20 19:28:46, sclittle wrote: > This could possibly destroy the URLRequestContext for a secure proxy check > that's in progress if the user toggles Data Saver on and off repeatedly really > fast. The URLRequestContext would also be destroyed prematurely if the DRPConfig > is destroyed before the URLFetcher gets destroyed. > > I'd suggest that you create your own SecureProxyCheckURLRequestContextGetter > class in this file that takes ownership of the URLRequestContext and the > HttpNetworkLayer, and lazily initialize that URLRequestContextGetter, having > DRPConfig hold on to a scoped_refptr of it, and reuse it for later secure proxy > checks. Done except that DRPConfig holds on the scoped_ptr of SecureProxyChecker instead of scoped_refptr https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:433: url_request_context_getter_->GetURLRequestContext()); On 2015/03/20 19:28:46, sclittle wrote: > I think it's only safe to use the profile's URLRequestContextGetter > GetURLRequestContext() on the UI thread, see: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... GetURLRequestContext() can only be used on IO thread. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_... https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:435: const net::HttpNetworkSession::Params* params_original = On 2015/03/20 19:28:46, sclittle wrote: > This variable is unnecessary. You could just have: > > DCHECK(url_request_context->GetNetworkSessionParams()); > net::HttpNetworkSession::Params params = > *url_request_context->GetNetworkSessionParams(); > > or something like that. Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:446: http_network_layer.reset(new net::HttpNetworkLayer(network_session.get())); On 2015/03/20 19:28:46, sclittle wrote: > You could get rid of the network_session variable and just have "new > net::HttpNetworkLayer(new net::HttpNetworkSession(params))" Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:452: getter_secure_proxy_check_ = new net::TrivialURLRequestContextGetter( On 2015/03/20 19:28:46, sclittle wrote: > You're constructing a new URLRequestContext and URLRequestContextGetter for > every secure proxy check; why not just lazily construct it once and re-use it? Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:231: scoped_ptr<net::HttpNetworkLayer> http_network_layer; On 2015/03/20 19:28:46, sclittle wrote: > needs a "_" at the end. Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:15: #include "net/url_request/url_request_context_getter.h" On 2015/03/20 19:28:46, sclittle wrote: > Unnecessary include. Done. https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:43: net::URLRequestContextGetter* request_context_getter, On 2015/03/20 19:28:46, sclittle wrote: > I think the net::URLRequestContextGetter from the BrowserContext is only allowed > to be used on the UI thread. See: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > What you probably have to do is call GetURLRequestContext on the UI thread, then > pass the URLRequestContext to the DRPConfig on the IO thread. Right, GetRequestContext() can only be called on UI thread (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) GetURLRequestContext() can only be called on IO thread (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...) https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/981633002/diff/100001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:96: new net::TestURLRequestContext(true)); On 2015/03/24 15:39:12, bengr wrote: > Indentation. Done.
https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h (right): https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h:10: #include "net/url_request/url_request_context_getter.h" Could this just be a forward declaration instead? https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h (right): https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:23: class URLRequestContextGetter; Remove unused forward declaration. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:57: net::HttpNetworkSession::Params params_modified, Change to const reference. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:58: scoped_refptr<net::URLRequestContextGetter> url_request_context_getter Move the end-paren at the start of the next line up to this line. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:59: ) : url_request_context_(url_request_context.release()) { s/.release()/.Pass()/ https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:70: void OnURLFetchComplete(const net::URLFetcher* source) override { Maybe add DCHECKs to ensure that all the methods of this class run on the IO thread? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:82: net::URLFetcher* fetcher = net::URLFetcher::Create( Could you just use |fetcher_|, and remove this |fetcher| variable? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:140: url_request_context_getter_ = request_context_getter; Move this into the initializer list. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:340: ui_task_runner_->PostTask( Could you just run this on the IO thread? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:469: ui_task_runner_->PostTask( Could you just run StartSecureProxyCheck on the IO thread? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:522: void DataReductionProxyConfig::StartSecureProxyCheck() { Can this method be run on the IO thread instead? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:526: if (data_reduction_proxy_service_) { You don't actually depend on drp_service_ anymore, so you can remove this if statement https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:539: )); Move end-parens up. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:597: ); Move end-paren up https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:18: #include "net/http/http_network_layer.h" Remove unnecessary include. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:21: #include "net/url_request/url_fetcher_delegate.h" Remove unnecessary include. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:22: #include "net/url_request/url_request_context_getter.h" Remove unnecessary include. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:36: class URLFetcher; Alphabetize these. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:42: typedef base::Callback<void(const std::string&, const net::URLRequestStatus&)> Add an include for <string> https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:45: class SecureProxyChecker; Alphabetize these. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:227: scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; Move these below the private methods. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:308: base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_; Can you remove the DRPService from DRPConfig? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:12: #include "net/url_request/url_request_context_getter.h" Could this just be a forward declaration? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:96: scoped_refptr<net::URLRequestContextGetter> request_context_getter, nit: maybe change to raw pointer for consistency with other DRPConfig constructors? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:134: EXPECT_CALL(*(config()), SecureProxyCheck(_, _)) Could this just be *config() instead of *(config())? https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:162: test_context_-> request_context_getter(), remove space in the middle https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:99: new net::TestURLRequestContextGetter( Change to new net::TestURLRequestContextGetter(task_runner), which will create a default TestURLRequestContext, since you're just passing in a default TestURLRequestContext anyways. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:200: net::HttpNetworkSession::Params* params = This memory gets leaked. https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... net/url_request/url_request_test_util.h:75: const_cast<HttpNetworkSession::Params*>(params)); You should never have to cast a const pointer to a non-const pointer. Instead you could pass in a scoped_ptr<HttpNetworkSession::Params> and take ownership of it. Also, move the implementation of this method into the .cc file.
https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:200: net::HttpNetworkSession::Params* params = On 2015/04/02 00:07:00, sclittle wrote: > This memory gets leaked. Oops, nevermind, it doesn't. Disregard this.
ptal https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h (right): https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h:10: #include "net/url_request/url_request_context_getter.h" On 2015/04/02 00:06:58, sclittle wrote: > Could this just be a forward declaration instead? Done. https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h (right): https://codereview.chromium.org/981633002/diff/120001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:23: class URLRequestContextGetter; On 2015/04/02 00:06:59, sclittle wrote: > Remove unused forward declaration. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:57: net::HttpNetworkSession::Params params_modified, On 2015/04/02 00:06:59, sclittle wrote: > Change to const reference. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:58: scoped_refptr<net::URLRequestContextGetter> url_request_context_getter On 2015/04/02 00:06:59, sclittle wrote: > Move the end-paren at the start of the next line up to this line. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:59: ) : url_request_context_(url_request_context.release()) { On 2015/04/02 00:06:59, sclittle wrote: > s/.release()/.Pass()/ Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:70: void OnURLFetchComplete(const net::URLFetcher* source) override { On 2015/04/02 00:06:59, sclittle wrote: > Maybe add DCHECKs to ensure that all the methods of this class run on the IO > thread? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:82: net::URLFetcher* fetcher = net::URLFetcher::Create( On 2015/04/02 00:06:59, sclittle wrote: > Could you just use |fetcher_|, and remove this |fetcher| variable? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:140: url_request_context_getter_ = request_context_getter; On 2015/04/02 00:06:59, sclittle wrote: > Move this into the initializer list. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:340: ui_task_runner_->PostTask( On 2015/04/02 00:06:59, sclittle wrote: > Could you just run this on the IO thread? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:469: ui_task_runner_->PostTask( On 2015/04/02 00:06:59, sclittle wrote: > Could you just run StartSecureProxyCheck on the IO thread? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:522: void DataReductionProxyConfig::StartSecureProxyCheck() { On 2015/04/02 00:06:59, sclittle wrote: > Can this method be run on the IO thread instead? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:526: if (data_reduction_proxy_service_) { On 2015/04/02 00:06:59, sclittle wrote: > You don't actually depend on drp_service_ anymore, so you can remove this if > statement Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:539: )); On 2015/04/02 00:06:59, sclittle wrote: > Move end-parens up. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:597: ); On 2015/04/02 00:06:59, sclittle wrote: > Move end-paren up Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:18: #include "net/http/http_network_layer.h" On 2015/04/02 00:06:59, sclittle wrote: > Remove unnecessary include. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:21: #include "net/url_request/url_fetcher_delegate.h" On 2015/04/02 00:07:00, sclittle wrote: > Remove unnecessary include. Not done. This is required. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:22: #include "net/url_request/url_request_context_getter.h" On 2015/04/02 00:07:00, sclittle wrote: > Remove unnecessary include. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:36: class URLFetcher; On 2015/04/02 00:06:59, sclittle wrote: > Alphabetize these. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:42: typedef base::Callback<void(const std::string&, const net::URLRequestStatus&)> On 2015/04/02 00:06:59, sclittle wrote: > Add an include for <string> Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:45: class SecureProxyChecker; On 2015/04/02 00:06:59, sclittle wrote: > Alphabetize these. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:227: scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; On 2015/04/02 00:06:59, sclittle wrote: > Move these below the private methods. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:308: base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_; On 2015/04/02 00:06:59, sclittle wrote: > Can you remove the DRPService from DRPConfig? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:12: #include "net/url_request/url_request_context_getter.h" On 2015/04/02 00:07:00, sclittle wrote: > Could this just be a forward declaration? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:96: scoped_refptr<net::URLRequestContextGetter> request_context_getter, On 2015/04/02 00:07:00, sclittle wrote: > nit: maybe change to raw pointer for consistency with other DRPConfig > constructors? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:134: EXPECT_CALL(*(config()), SecureProxyCheck(_, _)) On 2015/04/02 00:07:00, sclittle wrote: > Could this just be *config() instead of *(config())? Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:162: test_context_-> request_context_getter(), On 2015/04/02 00:07:00, sclittle wrote: > remove space in the middle Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:99: new net::TestURLRequestContextGetter( On 2015/04/02 00:07:00, sclittle wrote: > Change to new net::TestURLRequestContextGetter(task_runner), which will create a > default TestURLRequestContext, since you're just passing in a default > TestURLRequestContext anyways. Done. https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:200: net::HttpNetworkSession::Params* params = On 2015/04/02 00:08:35, sclittle wrote: > On 2015/04/02 00:07:00, sclittle wrote: > > This memory gets leaked. > > Oops, nevermind, it doesn't. Disregard this. Done. https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... net/url_request/url_request_test_util.h:75: const_cast<HttpNetworkSession::Params*>(params)); On 2015/04/02 00:07:00, sclittle wrote: > You should never have to cast a const pointer to a non-const pointer. > > Instead you could pass in a scoped_ptr<HttpNetworkSession::Params> and take > ownership of it. > > Also, move the implementation of this method into the .cc file. Done. Except not moved into the .cc file to stay consistent with other set* functions in this class.
https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:21: #include "net/url_request/url_fetcher_delegate.h" On 2015/04/02 23:21:19, tbansal1 wrote: > On 2015/04/02 00:07:00, sclittle wrote: > > Remove unnecessary include. > > Not done. This is required. Could you move it to the .cc file then? I don't see any mention of URLFetcherDelegate in this .h file. https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... net/url_request/url_request_test_util.h:75: const_cast<HttpNetworkSession::Params*>(params)); On 2015/04/02 23:21:20, tbansal1 wrote: > On 2015/04/02 00:07:00, sclittle wrote: > > You should never have to cast a const pointer to a non-const pointer. > > > > Instead you could pass in a scoped_ptr<HttpNetworkSession::Params> and take > > ownership of it. > > > > Also, move the implementation of this method into the .cc file. > > Done. Except not moved into the .cc file to stay consistent with other set* > functions in this class. Could you pass in a scoped_ptr instead, so that it's obvious that the TestURLRequestContext takes ownership of the params? Also, the implementation here should be moved to the .cc file instead of being inlined in the header file here: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:52: class SecureProxyChecker : public net::URLFetcherDelegate { Add a comment describing what this class does. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:62: http_network_layer_.reset(new net::HttpNetworkLayer( nit: Move this initialization of |http_network_layer_| into the initializer list above. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:74: DCHECK(io_task_runner_->BelongsToCurrentThread()); nit: move the thread DCHECK to the very start of the method. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:515: void DataReductionProxyConfig::StartSecureProxyCheck() { It seems weird to have both a StartSecureProxyCheck method and a SecureProxyCheck method. Could the contents of the method SecureProxyCheck() be merged into this method, and could you move the logic for creating the SecureProxyCheck URLRequestContext into the constructor of SecureProxyChecker? https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:91: scoped_refptr<base::SingleThreadTaskRunner> task_runner = Could you remove this |task_runner| variable and just use base::MessageLoopProxy::current() inline below? https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:94: scoped_ptr<net::TestURLRequestContext> test_request_context( This TestURLRequestContext doesn't seem to be used anywhere; could it be removed?
ptal https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:21: #include "net/url_request/url_fetcher_delegate.h" On 2015/04/03 02:06:56, sclittle wrote: > On 2015/04/02 23:21:19, tbansal1 wrote: > > On 2015/04/02 00:07:00, sclittle wrote: > > > Remove unnecessary include. > > > > Not done. This is required. > > Could you move it to the .cc file then? I don't see any mention of > URLFetcherDelegate in this .h file. Done. https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... File net/url_request/url_request_test_util.h (right): https://codereview.chromium.org/981633002/diff/120001/net/url_request/url_req... net/url_request/url_request_test_util.h:75: const_cast<HttpNetworkSession::Params*>(params)); On 2015/04/03 02:06:56, sclittle wrote: > On 2015/04/02 23:21:20, tbansal1 wrote: > > On 2015/04/02 00:07:00, sclittle wrote: > > > You should never have to cast a const pointer to a non-const pointer. > > > > > > Instead you could pass in a scoped_ptr<HttpNetworkSession::Params> and take > > > ownership of it. > > > > > > Also, move the implementation of this method into the .cc file. > > > > Done. Except not moved into the .cc file to stay consistent with other set* > > functions in this class. > > Could you pass in a scoped_ptr instead, so that it's obvious that the > TestURLRequestContext takes ownership of the params? > > Also, the implementation here should be moved to the .cc file instead of being > inlined in the header file here: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... Not moved to C++ file. There are lot of other code thats making use of the functions in this class. Probably should be done in a separate cleanup-CL. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:52: class SecureProxyChecker : public net::URLFetcherDelegate { On 2015/04/03 02:06:57, sclittle wrote: > Add a comment describing what this class does. Done. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:62: http_network_layer_.reset(new net::HttpNetworkLayer( On 2015/04/03 02:06:56, sclittle wrote: > nit: Move this initialization of |http_network_layer_| into the initializer list > above. Obsolete. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:74: DCHECK(io_task_runner_->BelongsToCurrentThread()); On 2015/04/03 02:06:57, sclittle wrote: > nit: move the thread DCHECK to the very start of the method. Done. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:515: void DataReductionProxyConfig::StartSecureProxyCheck() { On 2015/04/03 02:06:57, sclittle wrote: > It seems weird to have both a StartSecureProxyCheck method and a > SecureProxyCheck method. > > Could the contents of the method SecureProxyCheck() be merged into this method, > and could you move the logic for creating the SecureProxyCheck URLRequestContext > into the constructor of SecureProxyChecker? Done. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:91: scoped_refptr<base::SingleThreadTaskRunner> task_runner = On 2015/04/03 02:06:57, sclittle wrote: > Could you remove this |task_runner| variable and just use > base::MessageLoopProxy::current() inline below? Done. https://codereview.chromium.org/981633002/diff/140001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:94: scoped_ptr<net::TestURLRequestContext> test_request_context( On 2015/04/03 02:06:57, sclittle wrote: > This TestURLRequestContext doesn't seem to be used anywhere; could it be > removed? Done.
https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:58: scoped_refptr<net::URLRequestContextGetter> url_request_context_getter, You don't need to pass in |url_request_context_getter|, you could just use the |io_task_runner_| as the network task runner for your new TrivialURLRequestContextGetter. https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:155: DCHECK(url_request_context_getter_); nit: Move this DCHECK to the top of the constructor, such that it matches the order of method parameters. https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:232: FetcherResponseCallback fetcher_callback); indentation https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:44: // Constructs statistics prefs. This should not be called if a valid Did you accidentally change this comment to say "statistics prefs"?
Thanks for the comments. ptal! https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:58: scoped_refptr<net::URLRequestContextGetter> url_request_context_getter, On 2015/04/06 17:19:20, sclittle wrote: > You don't need to pass in |url_request_context_getter|, you could just use the > |io_task_runner_| as the network task runner for your new > TrivialURLRequestContextGetter. Done. https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:155: DCHECK(url_request_context_getter_); On 2015/04/06 17:19:20, sclittle wrote: > nit: Move this DCHECK to the top of the constructor, such that it matches the > order of method parameters. Done. https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:232: FetcherResponseCallback fetcher_callback); On 2015/04/06 17:19:20, sclittle wrote: > indentation Done. https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/981633002/diff/160001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:44: // Constructs statistics prefs. This should not be called if a valid On 2015/04/06 17:19:20, sclittle wrote: > Did you accidentally change this comment to say "statistics prefs"? Done.
components/data_reduction_proxy/ and chrome/browser/net/spdyproxy/ LGTM with nits. https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:59: : io_task_runner_(io_task_runner) { Indentation seems weird, could you run git cl format? https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:27: #include "net/url_request/url_request_context_getter.h" nit: I don't think this include is necessary, the forward declaration in the .h file is probably enough, since this file just holds on to a scoped_refptr of the URLRequestContextGetter, and doesn't call any methods on it.
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke for chrome/browser/profiles/* and net/url_request/* https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:59: : io_task_runner_(io_task_runner) { On 2015/04/08 18:02:06, sclittle wrote: > Indentation seems weird, could you run git cl format? Done. This changed whitespace at other places too. https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/981633002/diff/180001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:27: #include "net/url_request/url_request_context_getter.h" On 2015/04/08 18:02:06, sclittle wrote: > nit: I don't think this include is necessary, the forward declaration in the .h > file is probably enough, since this file just holds on to a scoped_refptr of the > URLRequestContextGetter, and doesn't call any methods on it. This is required because URLRequestContextGetter is ref counted, so passing a pointer to it calls Release(). Not having the include, then gives compilation error on line 61.
profiles/ and net/ LGTM
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps200001 (title: "Fixed formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps220001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/220001
The CQ bit was unchecked by tbansal@chromium.org
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps240001 (title: "Fix missing header file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/240001
The CQ bit was unchecked by tbansal@chromium.org
tbansal@chromium.org changed reviewers: + sgurun@chromium.org
sgurun@chromium.org: Please review changes in android_webview/*
sgurun@chromium.org: Please review changes in android_webview/*
sgurun@chromium.org: Please review changes in android_webview/*
On 2015/04/08 22:22:59, tbansal1 wrote: > mailto:sgurun@chromium.org: Please review changes in > > android_webview/* lgtm. interesting stuff.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps260001 (title: "Update data reduction proxy init in android webview")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps280001 (title: "Rebased again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/280001
The CQ bit was unchecked by tbansal@chromium.org
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps300001 (title: "Fixed minor bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/300001
The CQ bit was unchecked by tbansal@chromium.org
https://codereview.chromium.org/981633002/diff/300001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/981633002/diff/300001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:29: net::URLRequestContextGetter* request_context_getter, nit: name the variable: url_request_context_getter https://codereview.chromium.org/981633002/diff/300001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/300001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:79: DCHECK(io_task_runner_->BelongsToCurrentThread()); Ideally, no classes that are owned by DRPIOData would have any threading code. Instead, imho, tasks should be posted with MessageLoop::Current().
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps320001 (title: "Rebased on jeremyim@ CL which simplifies everything")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/320001
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
tbansal@chromium.org changed reviewers: + jeremyim@chromium.org
ptal https://codereview.chromium.org/981633002/diff/300001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/981633002/diff/300001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:29: net::URLRequestContextGetter* request_context_getter, On 2015/04/10 17:11:43, bengr wrote: > nit: name the variable: url_request_context_getter obsolete. https://codereview.chromium.org/981633002/diff/300001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/300001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:79: DCHECK(io_task_runner_->BelongsToCurrentThread()); On 2015/04/10 17:11:43, bengr wrote: > Ideally, no classes that are owned by DRPIOData would have any threading code. > Instead, imho, tasks should be posted with MessageLoop::Current(). There is no Post in drp_config but I am still keeping the DCHECKs for now.
https://codereview.chromium.org/981633002/diff/320001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/320001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:38: class URLRequest; Remove duplicate forward declarations
Done. Submitting now. https://codereview.chromium.org/981633002/diff/320001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/320001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:38: class URLRequest; On 2015/04/14 23:25:47, sclittle wrote: > Remove duplicate forward declarations Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, sclittle@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps340001 (title: "Removed duplicate forward declarations")
lgtm fwiw
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/340001
Some comments for posterity. They'll have to be addressed in a future CL if this commits. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:340: base::Unretained(this))); Use of base::Unretained should be commented as to why it's safe. That being said, both calls to SecureProxyCheck take the same parameters, so why are they passed in? https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:522: url_request_context_getter_->GetURLRequestContext(), io_task_runner_)); It makes more sense for this to be in InitializeOnIOThread https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:19: #include "net/http/http_network_layer.h" Is this necessary? https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:97: base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service); Remove this method, as data_reduction_proxy_service_ is no longer used. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:277: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; Remove this member. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:302: base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_; Remove this member. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:14: #include "net/base/load_flags.h" Can this be removed?
The CQ bit was unchecked by tbansal@chromium.org
thanks for the comments. ptal https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:340: base::Unretained(this))); On 2015/04/15 00:22:54, jeremyim wrote: > Use of base::Unretained should be commented as to why it's safe. > > That being said, both calls to SecureProxyCheck take the same parameters, so why > are they passed in? Added the comment. The parameters are necessary so we can change the callback in the tests. Not sure of a better way to do this. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:522: url_request_context_getter_->GetURLRequestContext(), io_task_runner_)); On 2015/04/15 00:22:54, jeremyim wrote: > It makes more sense for this to be in InitializeOnIOThread Done. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:19: #include "net/http/http_network_layer.h" On 2015/04/15 00:22:54, jeremyim wrote: > Is this necessary? Done. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:97: base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service); On 2015/04/15 00:22:54, jeremyim wrote: > Remove this method, as data_reduction_proxy_service_ is no longer used. Done. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:277: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; On 2015/04/15 00:22:54, jeremyim wrote: > Remove this member. Done. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:302: base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_; On 2015/04/15 00:22:54, jeremyim wrote: > Remove this member. Done. https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/981633002/diff/340001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:14: #include "net/base/load_flags.h" On 2015/04/15 00:22:54, jeremyim wrote: > Can this be removed? Done.
lgtm with a couple of nits. https://codereview.chromium.org/981633002/diff/360001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/981633002/diff/360001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:129: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, nit: ui_task_runner is no longer used and can be removed from the constructor. https://codereview.chromium.org/981633002/diff/360001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:337: // |secure_proxy_checker_| is shorter than of |this|. nit: It is safe to use base::Unretained here, since it gets executed synchronously on the IO thread, and |this| outlives |secure_proxy_checker_|. https://codereview.chromium.org/981633002/diff/360001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:469: // |secure_proxy_checker_| is shorter than of |this|. nit: It is safe to use base::Unretained here, since it gets executed synchronously on the IO thread, and |this| outlives |secure_proxy_checker_|.
Addressed comments, submitting now.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, sclittle@chromium.org, mmenke@chromium.org, jeremyim@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps380001 (title: "Addressed comments, submitting now.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/652eabf1141c00594aa6e9ed81beb980aec89198 Cr-Commit-Position: refs/heads/master@{#325302}
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/afc53166cb6a0ed561f416403ab68261eae4427b Cr-Commit-Position: refs/heads/master@{#325464}
Message was sent while issue was closed.
On 2015/04/16 17:26:49, I haz the power (commit-bot) wrote: > Patchset 20 (id:??) landed as > https://crrev.com/afc53166cb6a0ed561f416403ab68261eae4427b > Cr-Commit-Position: refs/heads/master@{#325464} This is breaking all the tests again: http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... Reverting. Talk with boliu@ to see if you can get this sorted.
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.chromium.org/1089443003/ by dfalcantara@chromium.org. The reason for reverting is: This is breaking all the tests again: http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%....
Message was sent while issue was closed.
On 2015/04/16 22:23:09, dfalcantara wrote: > A revert of this CL (patchset #20 id:380001) has been created in > https://codereview.chromium.org/1089443003/ by mailto:dfalcantara@chromium.org. > > The reason for reverting is: This is breaking all the tests again: > http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%.... From the failure logs: 71bb4: 04-16 18:14:15.459 2736 2785 F chromium: [FATAL:aw_url_request_context_getter.cc(127)] Check failed: set_protocol. signal 6 (SIGABRT) at 0x00000b63 (code=-6), thread 2949 (Chrome_IOThread) pid: 2915, tid: 2949, name: Chrome_IOThread >>> org.chromium.android_webview.shell <<< signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- r0 00000000 r1 00000b85 r2 00000006 r3 00000000 r4 00000006 r5 00000000 r6 00000b85 r7 0000010c r8 40110384 r9 7c3a024c sl 00000000 fp 787af9f4 ip 7c39fd6c sp 7c39fbe8 lr 400d4fe5 pc 400e3f90 Stack Trace: RELADDR FUNCTION FILE:LINE 00021f90 tgkill+12 /system/lib/libc.so 00012fe1 pthread_kill+48 /system/lib/libc.so 000131f5 raise+10 /system/lib/libc.so 00011f2b <unknown> /system/lib/libc.so 00021844 abort+4 /system/lib/libc.so 003654c5 base::debug::BreakDebugger()+8 libgcc2.c:? 003767d1 logging::LogMessage::~LogMessage()+388 libgcc2.c:? 002765c9 android_webview::AwURLRequestContextGetter::InitializeURLRequestContext()+1736 libgcc2.c:? 00276a13 android_webview::AwURLRequestContextGetter::GetURLRequestContext()+106 libgcc2.c:? 0203e8ed data_reduction_proxy::DataReductionProxyConfig::InitializeOnIOThread(net::URLRequestContextGetter*)+64 libgcc2.c:? 020409df data_reduction_proxy::DataReductionProxyIOData::InitializeOnIOThread()+90 libgcc2.c:? 003663b7 base::debug::TaskAnnotator::RunTask(char const*, char const*, base::PendingTask const&)+446 libgcc2.c:? 0037b515 base::MessageLoop::RunTask(base::PendingTask const&)+140 libgcc2.c:? 0037b599 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)+32 libgcc2.c:? 0037ca27 base::MessageLoop::DoWork()+110 libgcc2.c:? 0035641d base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+68 libgcc2.c:? 0037c3eb base::MessageLoop::RunHandler()+82 libgcc2.c:? 0038ca2d base::RunLoop::Run()+28 libgcc2.c:? 0037afb1 base::MessageLoop::Run()+12 libgcc2.c:? 009d5447 content::BrowserThreadImpl::IOThreadRun(base::MessageLoop*)+18 libgcc2.c:? 009d551d content::BrowserThreadImpl::Run(base::MessageLoop*)+132 browser_thread_impl.cc:? 003a1ff1 base::Thread::ThreadMain()+348 libgcc2.c:? 0039e85b base::(anonymous namespace)::ThreadFunc(void*)+110 platform_thread_posix.cc:? 0000d170 __thread_entry+72 /system/lib/libc.so 0000d308 pthread_create+240 /system/lib/libc.so
Message was sent while issue was closed.
Thanks for the log, this is very helpful. On Thu, Apr 16, 2015 at 3:27 PM <dfalcantara@chromium.org> wrote: > On 2015/04/16 22:23:09, dfalcantara wrote: > > A revert of this CL (patchset #20 id:380001) has been created in > > https://codereview.chromium.org/1089443003/ by > mailto:dfalcantara@chromium.org. > > > The reason for reverting is: This is breaking all the tests again: > > > http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... > . > > From the failure logs: > > 71bb4: 04-16 18:14:15.459 2736 2785 F chromium: > [FATAL:aw_url_request_context_getter.cc(127)] Check failed: set_protocol. > > signal 6 (SIGABRT) at 0x00000b63 (code=-6), thread 2949 (Chrome_IOThread) > pid: 2915, tid: 2949, name: Chrome_IOThread >>> > org.chromium.android_webview.shell <<< > signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- > r0 00000000 r1 00000b85 r2 00000006 r3 00000000 > r4 00000006 r5 00000000 r6 00000b85 r7 0000010c > r8 40110384 r9 7c3a024c sl 00000000 fp 787af9f4 > ip 7c39fd6c sp 7c39fbe8 lr 400d4fe5 pc 400e3f90 > > Stack Trace: > RELADDR FUNCTION > FILE:LINE > 00021f90 tgkill+12 > /system/lib/libc.so > 00012fe1 pthread_kill+48 > /system/lib/libc.so > 000131f5 raise+10 > /system/lib/libc.so > 00011f2b <unknown> > /system/lib/libc.so > 00021844 abort+4 > /system/lib/libc.so > 003654c5 base::debug::BreakDebugger()+8 > libgcc2.c:? > 003767d1 logging::LogMessage::~LogMessage()+388 > libgcc2.c:? > 002765c9 > > android_webview::AwURLRequestContextGetter::InitializeURLRequestContext()+1736 > libgcc2.c:? > 00276a13 > android_webview::AwURLRequestContextGetter::GetURLRequestContext()+106 > libgcc2.c:? > 0203e8ed > > data_reduction_proxy::DataReductionProxyConfig::InitializeOnIOThread(net::URLRequestContextGetter*)+64 > libgcc2.c:? > 020409df > data_reduction_proxy::DataReductionProxyIOData::InitializeOnIOThread()+90 > libgcc2.c:? > 003663b7 base::debug::TaskAnnotator::RunTask(char const*, char const*, > base::PendingTask const&)+446 libgcc2.c:? > 0037b515 base::MessageLoop::RunTask(base::PendingTask const&)+140 > libgcc2.c:? > 0037b599 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask > const&)+32 libgcc2.c:? > 0037ca27 base::MessageLoop::DoWork()+110 > libgcc2.c:? > 0035641d > base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+68 > libgcc2.c:? > 0037c3eb base::MessageLoop::RunHandler()+82 > libgcc2.c:? > 0038ca2d base::RunLoop::Run()+28 > libgcc2.c:? > 0037afb1 base::MessageLoop::Run()+12 > libgcc2.c:? > 009d5447 content::BrowserThreadImpl::IOThreadRun(base::MessageLoop*)+18 > libgcc2.c:? > 009d551d content::BrowserThreadImpl::Run(base::MessageLoop*)+132 > browser_thread_impl.cc:? > 003a1ff1 base::Thread::ThreadMain()+348 > libgcc2.c:? > 0039e85b base::(anonymous namespace)::ThreadFunc(void*)+110 > platform_thread_posix.cc:? > 0000d170 __thread_entry+72 > /system/lib/libc.so > 0000d308 pthread_create+240 > /system/lib/libc.so > > https://codereview.chromium.org/981633002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
So turns out the issue is the only cq bot that runs android tests does *not* turn on DCHECKs: crbug.com/477834 So this made past cq, and blew up on the debug bot on main waterfall. That particular stack is from android webview tests. There are other failures besides webview on the android bot though that are probably related: http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... We can chat offline about figure out where the other ones crashed.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, sclittle@chromium.org, jeremyim@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/981633002/#ps400001 (title: "Delayed calling GetURLRequestContext. This was causing webview tests to fail.")
The CQ bit was unchecked by tbansal@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/400001
The CQ bit was unchecked by boliu@chromium.org
lgtm fwiw
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981633002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/e9ceffc7d7758d47a45034b0b308b39eb7e515dc Cr-Commit-Position: refs/heads/master@{#325674} |
