Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
903213003:
Enable QUIC for proxies based on Finch config and command line switch. (Closed)
Description(1) Chrome data compression proxy should use QUIC only if
it is part of data compression QUIC field trial.
(2) If QUIC is disabled by command line switch or flag,
then QUIC should be disabled globally, including usage of
QUIC in data compression proxy.
(3) If Chrome is part of data compression QUIC field trial,
it does NOT enable QUIC for non-proxy URLs.
BUG=343579
Committed: https://crrev.com/ed0aeccb2df07b46ca69d7328d9fab6d5aae5f8e
Cr-Commit-Position: refs/heads/master@{#317232}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Followed on what we discussed. #
Total comments: 28
Patch Set 3 : Addressed comments and removed code duplication. #
Total comments: 16
Patch Set 4 : Addressed comments, added more tests #
Total comments: 16
Patch Set 5 : Addressed Ryan's comments. #Patch Set 6 : Added more tests. #Patch Set 7 : Fixed formatting #
Total comments: 37
Patch Set 8 : Addressed Ben's comments #
Total comments: 15
Patch Set 9 : Addressed all comments. #
Total comments: 10
Patch Set 10 : Addressed all comments. Added new field in IOThread globals #
Total comments: 4
Patch Set 11 : Removed enable_quic_for_data_reduction_proxy from http_network_session.h #
Total comments: 2
Patch Set 12 : Removed one tiny function. #
Total comments: 8
Patch Set 13 : Addressed comments. #
Total comments: 2
Patch Set 14 : Addressed Ryan's comments. #
Total comments: 2
Patch Set 15 : Changed variable name #Patch Set 16 : Moved back EnableQuic to UI thread #Patch Set 17 : Added TODO #
Total comments: 1
Patch Set 18 : Rebased and addressed comments #Patch Set 19 : Fixed error due to missing argument #
Total comments: 1
Patch Set 20 : Added comment #Messages
Total messages: 79 (13 generated)
tbansal@chromium.org changed reviewers: + bengr@chromium.org, rch@chromium.org
Initial suggestions sought: Would it be preferable to export the global state of QUIC from IOThread class to DRP?
https://codereview.chromium.org/903213003/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/1/chrome/browser/io_thread.cc#... chrome/browser/io_thread.cc:1231: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; Ah, I see. Hm, I'm not super in love this this. This allows the volume of QUIC traffic to be turned up it two different studies; only one of which is controlled by quic-dev (and hence is hooked into traffic-teams plans). You probably proposed this in your initial email I didn't grok it, but... Perhaps we should split the existing enable_quic support into enable_quic_to_origin and enable_quic_to_proxies. Your experiment could turn on the latter but not the former. Would something like that seem reasonable?
On 2015/02/06 22:23:53, Ryan Hamilton wrote: > https://codereview.chromium.org/903213003/diff/1/chrome/browser/io_thread.cc > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/903213003/diff/1/chrome/browser/io_thread.cc#... > chrome/browser/io_thread.cc:1231: "DataReductionProxyUseQUICAsPrimaryOrigin") == > "Enabled"; > Ah, I see. Hm, I'm not super in love this this. This allows the volume of QUIC > traffic to be turned up it two different studies; only one of which is > controlled by quic-dev (and hence is hooked into traffic-teams plans). > > You probably proposed this in your initial email I didn't grok it, but... > Perhaps we should split the existing enable_quic support into > enable_quic_to_origin and enable_quic_to_proxies. Your experiment could turn on > the latter but not the former. Would something like that seem reasonable? Yup, totally understand your concern. I did say that before but probably was not eloquent enough. I would think that it is preferable for "enable_quic_to_proxies" to be always true. If the user explicitly wants to use QUIC proxy then the finch should not disable it...right? This could be considered as user explicitly turning on QUIC flag in the chrome://flags. If we agree on that, then all we need to do is to ensure that QUIC works well when using QUIC proxy even if enable_quic global condition is false...right?
> Yup, totally understand your concern. I did say that before but probably was not > eloquent enough. That's far less likely than I was just being dense :> > I would think that it is preferable for "enable_quic_to_proxies" to be always > true. If the user explicitly wants to use QUIC proxy then the finch should not > disable it...right? This could be considered as user explicitly turning on QUIC > flag in the chrome://flags. > > If we agree on that, then all we need to do is to ensure that QUIC works well > when using QUIC proxy even if enable_quic global condition is false...right? Hm. I'm not sure we want to turn on QUIC (even just to proxies) by default. I would prefer enable_quic_to_proxies to be off unless the user is in a field trial which causes it to be turned on.
PTAL!
This should have tests. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; How about DataReductionProxyUseQUIC? And why does this not follow the same pattern as ShouldEnableQuic? At the very least, I don't think IOThread should know the name of our field trial. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.h:333: // Returns true if QUIC should be enabled for proxies. , either as a result of a field trial or a command line flag. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.h:336: base::StringPiece quic_trial_group); quic_proxy_trial_group? https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:762: DCHECK(session_->params().enable_quic_proxy); I'm confused. I thought we were going to DCHECK that enable_quic OR enable_quic_proxy is true. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:763: } Remove curly braces. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.h:302: // True if this network transaction is using QUIC proxy. This comment doesn't make sense.
https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; I think you need to check the --disable-quic flag? https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; s/QUIC/Quic/ when used like this. Do you already have an existing data compression proxy field trial study, or is this the first? https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.h:185: Optional<bool> enable_quic_proxy; I think you mean enable_quic_for_proxies, or some such? https://codereview.chromium.org/903213003/diff/20001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/20001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:105: "DataReductionProxyUseQUICAsPrimaryOrigin") == kEnabled; Hm. It's unfortunate that this file is duplicating the "can we speak quic to proxies" logic from IOThread. Is there any way to leverage IOThread? Could you look into IOThread, or HttpNetworkSessionParams? https://codereview.chromium.org/903213003/diff/20001/net/http/http_network_se... File net/http/http_network_session.h (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_network_se... net/http/http_network_session.h:116: bool enable_quic_proxy; enable_quic_for_proxies. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:678: ProxyServer::SCHEME_SOCKS4 | ProxyServer::SCHEME_SOCKS5); I think that if enable_quic_for_proxies is false, then you should remove SCHEME_QUIC from this list. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:762: DCHECK(session_->params().enable_quic_proxy); On 2015/02/07 01:48:48, bengr wrote: > I'm confused. I thought we were going to DCHECK that enable_quic OR > enable_quic_proxy is true. That was what I thought too.
ptal https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; On 2015/02/08 16:56:36, Ryan Hamilton wrote: > I think you need to check the --disable-quic flag? Done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; On 2015/02/08 16:56:36, Ryan Hamilton wrote: > I think you need to check the --disable-quic flag? Done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; On 2015/02/08 16:56:35, Ryan Hamilton wrote: > s/QUIC/Quic/ when used like this. Do you already have an existing data > compression proxy field trial study, or is this the first? Done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; On 2015/02/07 01:48:48, bengr wrote: > How about DataReductionProxyUseQUIC? > > And why does this not follow the same pattern as ShouldEnableQuic? At the very > least, I don't think IOThread should know the name of our field trial. Done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.h:185: Optional<bool> enable_quic_proxy; On 2015/02/08 16:56:36, Ryan Hamilton wrote: > I think you mean enable_quic_for_proxies, or some such? Done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.h:333: // Returns true if QUIC should be enabled for proxies. On 2015/02/07 01:48:48, bengr wrote: > , either as a result of a field trial or a command line flag. Done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.h:336: base::StringPiece quic_trial_group); On 2015/02/07 01:48:48, bengr wrote: > quic_proxy_trial_group? Done. https://codereview.chromium.org/903213003/diff/20001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/20001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:105: "DataReductionProxyUseQUICAsPrimaryOrigin") == kEnabled; On 2015/02/08 16:56:36, Ryan Hamilton wrote: > Hm. It's unfortunate that this file is duplicating the "can we speak quic to > proxies" logic from IOThread. Is there any way to leverage IOThread? Could you > look into IOThread, or HttpNetworkSessionParams? Done. https://codereview.chromium.org/903213003/diff/20001/net/http/http_network_se... File net/http/http_network_session.h (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_network_se... net/http/http_network_session.h:116: bool enable_quic_proxy; On 2015/02/08 16:56:36, Ryan Hamilton wrote: > enable_quic_for_proxies. Done. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:678: ProxyServer::SCHEME_SOCKS4 | ProxyServer::SCHEME_SOCKS5); On 2015/02/08 16:56:36, Ryan Hamilton wrote: > I think that if enable_quic_for_proxies is false, then you should remove > SCHEME_QUIC from this list. Done. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:762: DCHECK(session_->params().enable_quic_proxy); On 2015/02/07 01:48:48, bengr wrote: > I'm confused. I thought we were going to DCHECK that enable_quic OR > enable_quic_proxy is true. IIUC, this is stricter than the enable_quic OR enable_quic_proxy test. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:763: } On 2015/02/07 01:48:48, bengr wrote: > Remove curly braces. Done. https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/903213003/diff/20001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.h:302: // True if this network transaction is using QUIC proxy. On 2015/02/07 01:48:48, bengr wrote: > This comment doesn't make sense. Done.
ptal
https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:134: bool quic_enabled_for_proxies) { I weakly prefer adding EnableQUIC instead of adding this to Init...(). I'll leave it to you to decide. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:274: event_store_.get(), true); Do we also test with quic off? https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:110: if (command_line_origin_.empty() && IsIncludedInUseQUICFieldTrial() && I don't think you want to check if you're in the field trial here, since that's already done from the code in IOThread. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:292: // quic_globally_disabled_ = command_line.HasSwitch(switches::kDisableQuic); Remove dead code. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:309: LOG(WARNING) << "InitWithoutChecks origin.empty()=" << origin.empty() Remove this logging. If you need it, don't use WARNING unless you really mean it. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:543: return IsIncludedInUseQUICFieldTrial() && quic_enabled_for_proxies_ Don't check the field trial here. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:108: void SetIsQuicEnabledForProxies(bool quic_enabled_for_proxies); void EnableQUIC(bool enable) https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:272: bool is_quic_enabled_for_proxies_() const { bool IsQUICEnabled() const;
ptal! https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:134: bool quic_enabled_for_proxies) { On 2015/02/10 20:18:31, bengr wrote: > I weakly prefer adding EnableQUIC instead of adding this to Init...(). I'll > leave it to you to decide. Done. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:274: event_store_.get(), true); On 2015/02/10 20:18:31, bengr wrote: > Do we also test with quic off? Added in data_reduction_proxy_settings_unittest.cc. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:110: if (command_line_origin_.empty() && IsIncludedInUseQUICFieldTrial() && On 2015/02/10 20:18:31, bengr wrote: > I don't think you want to check if you're in the field trial here, since that's > already done from the code in IOThread. I need to: quic_enabled_for_proxies_ can be true if either is true (i) enable_quic is true OR (ii) chrome is part of DRP QUIC field trial. If enable_quic is true (may be because of QUIC field trial), we need to check for DRP QUIC field trial. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:292: // quic_globally_disabled_ = command_line.HasSwitch(switches::kDisableQuic); On 2015/02/10 20:18:31, bengr wrote: > Remove dead code. Done. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:309: LOG(WARNING) << "InitWithoutChecks origin.empty()=" << origin.empty() On 2015/02/10 20:18:31, bengr wrote: > Remove this logging. If you need it, don't use WARNING unless you really mean > it. Done. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:543: return IsIncludedInUseQUICFieldTrial() && quic_enabled_for_proxies_ On 2015/02/10 20:18:31, bengr wrote: > Don't check the field trial here. see above. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:108: void SetIsQuicEnabledForProxies(bool quic_enabled_for_proxies); On 2015/02/10 20:18:31, bengr wrote: > void EnableQUIC(bool enable) Done. https://codereview.chromium.org/903213003/diff/40001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:272: bool is_quic_enabled_for_proxies_() const { On 2015/02/10 20:18:31, bengr wrote: > bool IsQUICEnabled() const; Done.
Marking rch comments as done. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; On 2015/02/08 16:56:35, Ryan Hamilton wrote: > s/QUIC/Quic/ when used like this. Do you already have an existing data > compression proxy field trial study, or is this the first? This is the first. https://codereview.chromium.org/903213003/diff/20001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1240: "DataReductionProxyUseQUICAsPrimaryOrigin") == "Enabled"; On 2015/02/08 16:56:36, Ryan Hamilton wrote: > I think you need to check the --disable-quic flag? Done.
https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1241: return group; It looks like the only caller of this method is over in profile_impl_io_data.cc which immediately passes this on to IOThread::ShouldEnableQuicForProxies. If that's right, then I would nuke this method. Instead, I would create a one-arge ShouldEnableQuicForProxies method which passes this FindFullName into the two arg version. https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread... chrome/browser/io_thread.h:240: static std::string QuicFieldTrialName(); This returns the name of the *group* not the name of the field trial, right? https://codereview.chromium.org/903213003/diff/60001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc (right): https://codereview.chromium.org/903213003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:11: #include "chrome/browser/io_thread.h" Is this import needed? Looks like all the other changes in this file are whitespace. https://codereview.chromium.org/903213003/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/60001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:111: origin_ = net::ProxyServer::FromURI(kDefaultQuicOrigin, It's a bit confusing that there's a member named "origin" which is really a proxy server not an origin. But that predates this CL :> https://codereview.chromium.org/903213003/diff/60001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/60001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:766: DCHECK(session_->params().enable_quic_for_proxies); Is this the only place using_quic_proxy_ is used? If so, I'd move this DCHECK up to line 761. (And then remove this member) https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_end_to_end... File net/quic/quic_end_to_end_unittest.cc (right): https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_end_to_end... net/quic/quic_end_to_end_unittest.cc:86: params_.enable_quic_for_proxies = true; might be interesting to only set this to true for the tests which actually use the QUIC proxy. https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_network_tr... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_network_tr... net/quic/quic_network_transaction_unittest.cc:220: params_.enable_quic_for_proxies = true; Same comment about only doing this if we're planning to talk to a proxy. https://codereview.chromium.org/903213003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/903213003/diff/60001/net/url_request/url_requ... net/url_request/url_request_context_builder.h:167: void SetSpdyAndQuicEnabled(bool spdy_enabled, I don't see any changes to the caller of this method. Is that intentional? Perhaps this method does not need to change if you (flywheel) don't need to call it?
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@chromium.org: Please review changes in chrome/browser/profiles/* https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread... chrome/browser/io_thread.cc:1241: return group; On 2015/02/11 00:20:32, Ryan Hamilton wrote: > It looks like the only caller of this method is over in profile_impl_io_data.cc > which immediately passes this on to IOThread::ShouldEnableQuicForProxies. If > that's right, then I would nuke this method. Instead, I would create a one-arge > ShouldEnableQuicForProxies method which passes this FindFullName into the two > arg version. This was necessary for the adding tests in io_thread_unittest.cc If you feel strongly about it, I can think of some other way to write tests. https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/60001/chrome/browser/io_thread... chrome/browser/io_thread.h:240: static std::string QuicFieldTrialName(); On 2015/02/11 00:20:32, Ryan Hamilton wrote: > This returns the name of the *group* not the name of the field trial, right? Done. https://codereview.chromium.org/903213003/diff/60001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc (right): https://codereview.chromium.org/903213003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:11: #include "chrome/browser/io_thread.h" On 2015/02/11 00:20:32, Ryan Hamilton wrote: > Is this import needed? Looks like all the other changes in this file are > whitespace. Done. https://codereview.chromium.org/903213003/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/60001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:111: origin_ = net::ProxyServer::FromURI(kDefaultQuicOrigin, On 2015/02/11 00:20:32, Ryan Hamilton wrote: > It's a bit confusing that there's a member named "origin" which is really a > proxy server not an origin. But that predates this CL :> If other reviewers feel strongly about it, I volunteer to make changes in a separate CL :) https://codereview.chromium.org/903213003/diff/60001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/60001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:766: DCHECK(session_->params().enable_quic_for_proxies); On 2015/02/11 00:20:32, Ryan Hamilton wrote: > Is this the only place using_quic_proxy_ is used? If so, I'd move this DCHECK up > to line 761. (And then remove this member) Done. https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_end_to_end... File net/quic/quic_end_to_end_unittest.cc (right): https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_end_to_end... net/quic/quic_end_to_end_unittest.cc:86: params_.enable_quic_for_proxies = true; On 2015/02/11 00:20:32, Ryan Hamilton wrote: > might be interesting to only set this to true for the tests which actually use > the QUIC proxy. Done. https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_network_tr... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/903213003/diff/60001/net/quic/quic_network_tr... net/quic/quic_network_transaction_unittest.cc:220: params_.enable_quic_for_proxies = true; On 2015/02/11 00:20:32, Ryan Hamilton wrote: > Same comment about only doing this if we're planning to talk to a proxy. Done. https://codereview.chromium.org/903213003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/903213003/diff/60001/net/url_request/url_requ... net/url_request/url_request_context_builder.h:167: void SetSpdyAndQuicEnabled(bool spdy_enabled, On 2015/02/11 00:20:32, Ryan Hamilton wrote: > I don't see any changes to the caller of this method. Is that intentional? > Perhaps this method does not need to change if you (flywheel) don't need to call > it? Done.
Thanks for the comments, one more take please.
Added more tests. Ready for review.
https://codereview.chromium.org/903213003/diff/120001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/120001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1240: base::FieldTrialList::FindFullName(kQuicFieldTrialName); Indentation is off. https://codereview.chromium.org/903213003/diff/120001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1253: IsIncludedInUseQUICFieldTrial(); I'd rename as IsIncludedInQuicFieldTrial() also, data_reduction_proxy can be indented 4 from return, so this should fit on one line. https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:191: IOThread::ShouldEnableQuicForProxies(command_line, Why does command_line need to be passed in? https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:195: ->EnableQUIC(quic_enabled_for_proxies); Put the -> on the previous line and indent 4 from DataReduction... https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:155: params()->EnableQUIC(quic_enabled_for_proxies); DCHECK(params()) https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:90: // Sets DataReductionProxy params. Say that params() must be non-null. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:91: void EnableQUIC(bool quic_enabled_for_proxies); EnableQuic, as per rch's recommendation and rename the parameter as simply 'enable'. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:35: ~BadEntropyProvider() override {} Move close curly to new line. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:455: false, Indentation is off. See other examples in our tests. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:519: base::MessageLoop::current()->RunUntilIdle(); This can be part of the test harness. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:108: // Sets if QUIC is enabled for proxies. Also, may update the origin proxy What do you mean by 'may'? Can't you just say: // If true, uses QUIC instead of SPDY to connect to proxies that use TLS. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:274: bool IsQUICEnabled() const { IsQuicEnabled https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:339: bool quic_enabled_for_proxies_; quic_enabled_ https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:757: DCHECK(session_->params().enable_quic); Release builds won't have this statement. https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:762: DCHECK(session_->params().enable_quic_for_proxies); Release builds won't have this statement.
Thanks for comments. PTAL. https://codereview.chromium.org/903213003/diff/120001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/120001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1240: base::FieldTrialList::FindFullName(kQuicFieldTrialName); On 2015/02/11 23:57:27, bengr wrote: > Indentation is off. Done. https://codereview.chromium.org/903213003/diff/120001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1253: IsIncludedInUseQUICFieldTrial(); On 2015/02/11 23:57:27, bengr wrote: > I'd rename as IsIncludedInQuicFieldTrial() > Done. > also, data_reduction_proxy can be indented 4 from return, so this should fit on > one line. It looks like the cl formatter keeps moving it to indent 6. https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:191: IOThread::ShouldEnableQuicForProxies(command_line, On 2015/02/11 23:57:28, bengr wrote: > Why does command_line need to be passed in? This matches ShouldEnableQuic() header. This helps in testing (e.g., in io_thread_unittest.cc, command line is overridden). https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:195: ->EnableQUIC(quic_enabled_for_proxies); On 2015/02/11 23:57:27, bengr wrote: > Put the -> on the previous line and indent 4 from DataReduction... Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:155: params()->EnableQUIC(quic_enabled_for_proxies); On 2015/02/11 23:57:28, bengr wrote: > DCHECK(params()) Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:90: // Sets DataReductionProxy params. On 2015/02/11 23:57:28, bengr wrote: > Say that params() must be non-null. Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:91: void EnableQUIC(bool quic_enabled_for_proxies); On 2015/02/11 23:57:28, bengr wrote: > EnableQuic, as per rch's recommendation and rename the parameter as simply > 'enable'. Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:35: ~BadEntropyProvider() override {} On 2015/02/11 23:57:28, bengr wrote: > Move close curly to new line. I don't understand the comment. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:455: false, On 2015/02/11 23:57:28, bengr wrote: > Indentation is off. See other examples in our tests. Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:519: base::MessageLoop::current()->RunUntilIdle(); On 2015/02/11 23:57:28, bengr wrote: > This can be part of the test harness. removed since it was not useful for this test. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:108: // Sets if QUIC is enabled for proxies. Also, may update the origin proxy On 2015/02/11 23:57:28, bengr wrote: > What do you mean by 'may'? Can't you just say: > > // If true, uses QUIC instead of SPDY to connect to proxies that use TLS. Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:274: bool IsQUICEnabled() const { On 2015/02/11 23:57:28, bengr wrote: > IsQuicEnabled Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:339: bool quic_enabled_for_proxies_; On 2015/02/11 23:57:28, bengr wrote: > quic_enabled_ Done. https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:757: DCHECK(session_->params().enable_quic); On 2015/02/11 23:57:28, bengr wrote: > Release builds won't have this statement. I believe that's WAI. https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:762: DCHECK(session_->params().enable_quic_for_proxies); On 2015/02/11 23:57:28, bengr wrote: > Release builds won't have this statement. I believe that's WAI.
https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:191: IOThread::ShouldEnableQuicForProxies(command_line, On 2015/02/12 02:27:21, tbansal1 wrote: > On 2015/02/11 23:57:28, bengr wrote: > > Why does command_line need to be passed in? > > This matches ShouldEnableQuic() header. This helps in testing (e.g., in > io_thread_unittest.cc, command line is overridden). But ProfileImplIOData should not need to know how the decision is made by the DRP component. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:35: ~BadEntropyProvider() override {} On 2015/02/12 02:27:22, tbansal1 wrote: > On 2015/02/11 23:57:28, bengr wrote: > > Move close curly to new line. > > I don't understand the comment. I mean, style afaik is to do: ~BadEntopyProvider() override { } https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:757: DCHECK(session_->params().enable_quic); On 2015/02/12 02:27:22, tbansal1 wrote: > On 2015/02/11 23:57:28, bengr wrote: > > Release builds won't have this statement. > > I believe that's WAI. That can't be the case. In a release build, the code looks like this: if (using_quic_) if (proxy_info.is_quic()) { using_quic_ = true; using_quic_proxy_ = true; } https://codereview.chromium.org/903213003/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:116: if (command_line_origin_.empty() && IsIncludedInQuicFieldTrial() && I thought I commented on this but maybe I didn't. I don't think that we should guard this with a check that we're in the field trial. That's for the caller to determine. https://codereview.chromium.org/903213003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:545: return IsIncludedInQuicFieldTrial() && quic_enabled_ ? Likewise here, the quic origin should be return unconditionally if quic_enabled_.
https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1158: if (enable_quic) { Having quic disabled but enabled for proxies seems really weird. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1247: base::StringPiece quic_trial_group) { Why does this need to take the command line, or the quic trial group? We can get both from static methods. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:163: EXPECT_TRUE(params.enable_quic_for_proxies); I'm not following this test. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:166: nit: Remove extra blank line https://codereview.chromium.org/903213003/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:192: IOThread::QuicTrialGroup()); Calling IOThread methods on the UI threads, even static ones, is pretty hideous. I don't suppose we could just check globals->enable_quic_for_proxies on the IO thread and pass it to the DRP stuff there?
https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1247: base::StringPiece quic_trial_group) { On 2015/02/12 18:10:45, mmenke wrote: > Why does this need to take the command line, or the quic trial group? We can > get both from static methods. Agreed. I left the same comment.
Thanks for the comments. PTAL. One question for reviewers: Right now, I have function IsIncludedInQuicFieldTrial() in data_reduction_proxy_params.cc. This function returns true if Chrome is part of DRP QUIC field trial. If so, QUIC is enabled for all proxies (not just DRP). Given that this enables QUIC for all proxies, should this function stay in data_reduction_proxy_params.cc or should it be in IOThread or somewhere else in net stack? https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:191: IOThread::ShouldEnableQuicForProxies(command_line, On 2015/02/12 17:14:46, bengr wrote: > On 2015/02/12 02:27:21, tbansal1 wrote: > > On 2015/02/11 23:57:28, bengr wrote: > > > Why does command_line need to be passed in? > > > > This matches ShouldEnableQuic() header. This helps in testing (e.g., in > > io_thread_unittest.cc, command line is overridden). > > But ProfileImplIOData should not need to know how the decision is made by the > DRP component. Done. https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/120001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:35: ~BadEntropyProvider() override {} On 2015/02/12 17:14:46, bengr wrote: > On 2015/02/12 02:27:22, tbansal1 wrote: > > On 2015/02/11 23:57:28, bengr wrote: > > > Move close curly to new line. > > > > I don't understand the comment. > > I mean, style afaik is to do: > ~BadEntopyProvider() override { > } Not sure, see: https://code.google.com/p/chromium/codesearch#search/&q=override%5C%20%7B%7D%... Anyways, I copied this from data_reduction_proxy_bypass_protocol.cc, I can change there too if you prefer. https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:757: DCHECK(session_->params().enable_quic); On 2015/02/12 17:14:46, bengr wrote: > On 2015/02/12 02:27:22, tbansal1 wrote: > > On 2015/02/11 23:57:28, bengr wrote: > > > Release builds won't have this statement. > > > > I believe that's WAI. > > That can't be the case. In a release build, the code looks like this: > > if (using_quic_) > if (proxy_info.is_quic()) { > using_quic_ = true; > using_quic_proxy_ = true; > } Fixed. https://codereview.chromium.org/903213003/diff/120001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:762: DCHECK(session_->params().enable_quic_for_proxies); On 2015/02/12 02:27:22, tbansal1 wrote: > On 2015/02/11 23:57:28, bengr wrote: > > Release builds won't have this statement. > > I believe that's WAI. This is still okay. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1158: if (enable_quic) { On 2015/02/12 18:10:45, mmenke wrote: > Having quic disabled but enabled for proxies seems really weird. Acknowledged. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1247: base::StringPiece quic_trial_group) { On 2015/02/12 18:15:02, bengr wrote: > On 2015/02/12 18:10:45, mmenke wrote: > > Why does this need to take the command line, or the quic trial group? We can > > get both from static methods. > > Agreed. I left the same comment. Done. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:163: EXPECT_TRUE(params.enable_quic_for_proxies); On 2015/02/12 18:10:45, mmenke wrote: > I'm not following this test. When DRP field trial is enabled, we are making sure that QUIC is enabled only for proxies (and is disabled otherwise on non proxy transactions). https://codereview.chromium.org/903213003/diff/140001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:166: On 2015/02/12 18:10:45, mmenke wrote: > nit: Remove extra blank line Done. https://codereview.chromium.org/903213003/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:192: IOThread::QuicTrialGroup()); On 2015/02/12 18:10:45, mmenke wrote: > Calling IOThread methods on the UI threads, even static ones, is pretty hideous. > > I don't suppose we could just check globals->enable_quic_for_proxies on the IO > thread and pass it to the DRP stuff there? Fixed, now calling it on IO thread. https://codereview.chromium.org/903213003/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:116: if (command_line_origin_.empty() && IsIncludedInQuicFieldTrial() && On 2015/02/12 17:14:46, bengr wrote: > I thought I commented on this but maybe I didn't. I don't think that we should > guard this with a check that we're in the field trial. That's for the caller to > determine. Done. https://codereview.chromium.org/903213003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:545: return IsIncludedInQuicFieldTrial() && quic_enabled_ ? On 2015/02/12 17:14:46, bengr wrote: > Likewise here, the quic origin should be return unconditionally if > quic_enabled_. Done.
https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1269: IsIncludedInQuicFieldTrial(); Per earlier comment, do we really want to enable QUIC for proxies if QUIC itself is disabled? I could very easily see this violating fundamental assumptions in the QUIC logic. https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:163: EXPECT_TRUE(params.enable_quic_for_proxies); Could you explain how this works? I really don't know how the entropy provider you're using guarantees this result. https://codereview.chromium.org/903213003/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:465: IsIncludedInQuicFieldTrial()); The extra ShouldEnableQuicForProxies check seems weird, since the second check being true forces the first to be true... Suggest putting all this logic in IOThread, and setting another IOThreadGlobal based on the result (allow_quic_for_data_reduction_proxy or somesuch). That would centralize all chrome-side logic on the topic, and we could deal with all the cases there in one place. And then here you have: bool allow_quic_for_data_reduction_proxy = false; io_thread_globals->allow_quic_for_data_reduction_proxy.CopyToIfSet(&allow_quic_for_data_reduction_proxy); data_reduction_proxy_io_data()->Init(allow_quic_for_data_reduction_proxy);
Just responding to some comments quickly, while I work on the others. https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1269: IsIncludedInQuicFieldTrial(); On 2015/02/13 17:45:21, mmenke wrote: > Per earlier comment, do we really want to enable QUIC for proxies if QUIC itself > is disabled? I could very easily see this violating fundamental assumptions in > the QUIC logic. This was based on rch@ suggestion. There was a concern that DRP QUIC experiment may increase QUIC DIRECT traffic (which is not favorable because it is beyond quic-dev control). https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:163: EXPECT_TRUE(params.enable_quic_for_proxies); On 2015/02/13 17:45:21, mmenke wrote: > Could you explain how this works? I really don't know how the entropy provider > you're using guarantees this result. Constructor of FieldTrialList requires an entropy provider. However, CreateFieldTrial() does not use the entropy provider. (There are other methods in FieldTrialList that use entropy provider). So, creation of field trial is 100% guaranteed here.
Quick responses https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1269: IsIncludedInQuicFieldTrial(); On 2015/02/13 18:03:47, tbansal1 wrote: > On 2015/02/13 17:45:21, mmenke wrote: > > Per earlier comment, do we really want to enable QUIC for proxies if QUIC > itself > > is disabled? I could very easily see this violating fundamental assumptions > in > > the QUIC logic. > > This was based on rch@ suggestion. There was a concern that DRP QUIC experiment > may increase QUIC DIRECT traffic (which is not favorable because it is beyond > quic-dev control). Thanks for the followup! Happy to defer to rch here. https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:163: EXPECT_TRUE(params.enable_quic_for_proxies); On 2015/02/13 18:03:47, tbansal1 wrote: > On 2015/02/13 17:45:21, mmenke wrote: > > Could you explain how this works? I really don't know how the entropy > provider > > you're using guarantees this result. > > Constructor of FieldTrialList requires an entropy provider. However, > CreateFieldTrial() does not use the entropy provider. (There are other methods > in FieldTrialList that use entropy provider). So, creation of field trial is > 100% guaranteed here. Thanks for the explanation! Worth having a test where we only enable the DRP quic field trial, and make sure quic for proxies is also enabled (And the new settings I suggested)?
On 2015/02/13 18:03:47, tbansal1 wrote: > Just responding to some comments quickly, while I work on the others. > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > chrome/browser/io_thread.cc:1269: IsIncludedInQuicFieldTrial(); > On 2015/02/13 17:45:21, mmenke wrote: > > Per earlier comment, do we really want to enable QUIC for proxies if QUIC > itself > > is disabled? I could very easily see this violating fundamental assumptions > in > > the QUIC logic. > > This was based on rch@ suggestion. There was a concern that DRP QUIC experiment > may increase QUIC DIRECT traffic (which is not favorable because it is beyond > quic-dev control). > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > File chrome/browser/io_thread_unittest.cc (right): > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > chrome/browser/io_thread_unittest.cc:163: > EXPECT_TRUE(params.enable_quic_for_proxies); > On 2015/02/13 17:45:21, mmenke wrote: > > Could you explain how this works? I really don't know how the entropy > provider > > you're using guarantees this result. > > Constructor of FieldTrialList requires an entropy provider. However, > CreateFieldTrial() does not use the entropy provider. (There are other methods > in FieldTrialList that use entropy provider). So, creation of field trial is > 100% guaranteed here.
On 2015/02/13 18:09:44, mmenke wrote: > Quick responses > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > chrome/browser/io_thread.cc:1269: IsIncludedInQuicFieldTrial(); > On 2015/02/13 18:03:47, tbansal1 wrote: > > On 2015/02/13 17:45:21, mmenke wrote: > > > Per earlier comment, do we really want to enable QUIC for proxies if QUIC > > itself > > > is disabled? I could very easily see this violating fundamental assumptions > > in > > > the QUIC logic. > > > > This was based on rch@ suggestion. There was a concern that DRP QUIC > experiment > > may increase QUIC DIRECT traffic (which is not favorable because it is beyond > > quic-dev control). > > Thanks for the followup! Happy to defer to rch here. > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > File chrome/browser/io_thread_unittest.cc (right): > > https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... > chrome/browser/io_thread_unittest.cc:163: > EXPECT_TRUE(params.enable_quic_for_proxies); > On 2015/02/13 18:03:47, tbansal1 wrote: > > On 2015/02/13 17:45:21, mmenke wrote: > > > Could you explain how this works? I really don't know how the entropy > > provider > > > you're using guarantees this result. > > > > Constructor of FieldTrialList requires an entropy provider. However, > > CreateFieldTrial() does not use the entropy provider. (There are other methods > > in FieldTrialList that use entropy provider). So, creation of field trial is > > 100% guaranteed here. > > Thanks for the explanation! > > Worth having a test where we only enable the DRP quic field trial, and make sure > quic for proxies is also enabled (And the new settings I suggested)? Yes, there is a test in io_thread_unittest.cc that does that. I will add another test for the new settings.
https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:150: EXPECT_FALSE(params.quic_enable_connection_racing); Should check params.enable_quic_for_proxies here (I was thinking the next test did what this one is doing).
Thanks for comments. ptal. https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/io_threa... chrome/browser/io_thread_unittest.cc:163: EXPECT_TRUE(params.enable_quic_for_proxies); On 2015/02/13 18:09:44, mmenke wrote: > On 2015/02/13 18:03:47, tbansal1 wrote: > > On 2015/02/13 17:45:21, mmenke wrote: > > > Could you explain how this works? I really don't know how the entropy > > provider > > > you're using guarantees this result. > > > > Constructor of FieldTrialList requires an entropy provider. However, > > CreateFieldTrial() does not use the entropy provider. (There are other methods > > in FieldTrialList that use entropy provider). So, creation of field trial is > > 100% guaranteed here. > > Thanks for the explanation! > > Worth having a test where we only enable the DRP quic field trial, and make sure > quic for proxies is also enabled (And the new settings I suggested)? Done. https://codereview.chromium.org/903213003/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/903213003/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:465: IsIncludedInQuicFieldTrial()); On 2015/02/13 17:45:22, mmenke wrote: > The extra ShouldEnableQuicForProxies check seems weird, since the second check > being true forces the first to be true... > > Suggest putting all this logic in IOThread, and setting another IOThreadGlobal > based on the result (allow_quic_for_data_reduction_proxy or somesuch). That > would centralize all chrome-side logic on the topic, and we could deal with all > the cases there in one place. > > And then here you have: > > bool allow_quic_for_data_reduction_proxy = false; > io_thread_globals->allow_quic_for_data_reduction_proxy.CopyToIfSet(&allow_quic_for_data_reduction_proxy); > data_reduction_proxy_io_data()->Init(allow_quic_for_data_reduction_proxy); Done.
LGTM https://codereview.chromium.org/903213003/diff/180001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/180001/chrome/browser/io_threa... chrome/browser/io_thread.h:345: base::StringPiece quic_trial_group); These can now be removed from the header and moved into an anonymous namespace in io_thread.cc
https://codereview.chromium.org/903213003/diff/180001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/903213003/diff/180001/net/http/http_network_s... net/http/http_network_session.h:117: bool enable_quic_for_data_reduction_proxy; Where is this parameter being used? I didn't see it in any of the code in net/ but perhaps I missed it?
https://codereview.chromium.org/903213003/diff/180001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/903213003/diff/180001/net/http/http_network_s... net/http/http_network_session.h:117: bool enable_quic_for_data_reduction_proxy; On 2015/02/13 20:47:20, Ryan Hamilton wrote: > Where is this parameter being used? I didn't see it in any of the code in net/ > but perhaps I missed it? +1. This is also a layering violation. We shouldn't add any more of these.
New patchsets have been uploaded after l-g-t-m from mmenke@chromium.org
https://codereview.chromium.org/903213003/diff/180001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/903213003/diff/180001/net/http/http_network_s... net/http/http_network_session.h:117: bool enable_quic_for_data_reduction_proxy; On 2015/02/13 20:57:24, mmenke wrote: > On 2015/02/13 20:47:20, Ryan Hamilton wrote: > > Where is this parameter being used? I didn't see it in any of the code in net/ > > but perhaps I missed it? > > +1. This is also a layering violation. We shouldn't add any more of these. Removed it, it was used in unit tests. Now, testing in a different way.
https://codereview.chromium.org/903213003/diff/200001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/200001/chrome/browser/io_threa... chrome/browser/io_thread.h:186: Optional<bool> enable_quic_for_data_reduction_proxy; Can you remove this too?
https://codereview.chromium.org/903213003/diff/200001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/903213003/diff/200001/chrome/browser/io_threa... chrome/browser/io_thread.h:186: Optional<bool> enable_quic_for_data_reduction_proxy; On 2015/02/13 21:39:04, Ryan Hamilton wrote: > Can you remove this too? IIUC: No, this is part of IOThread::Globals, which is used in profile_impl_io_data.cc.
On 2015/02/13 22:04:13, tbansal1 wrote: > https://codereview.chromium.org/903213003/diff/200001/chrome/browser/io_thread.h > File chrome/browser/io_thread.h (right): > > https://codereview.chromium.org/903213003/diff/200001/chrome/browser/io_threa... > chrome/browser/io_thread.h:186: Optional<bool> > enable_quic_for_data_reduction_proxy; > On 2015/02/13 21:39:04, Ryan Hamilton wrote: > > Can you remove this too? > > IIUC: No, this is part of IOThread::Globals, which is used in > profile_impl_io_data.cc. Good point :>
https://codereview.chromium.org/903213003/diff/220001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/220001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1281: } I stared at these two methods for about 10 minutes trying to figure out the relationship between them. (I was feeling very dense). I think you can rewrite them to be a bit more clear and to reduce duplication (of the DRPP:IIIQFT call) as follows: bool IOThread::ShouldEnableQuicForDataReductionProxy( const base::CommandLine& command_line, base::StringPiece quic_trial_group) { if (command_line.HasSwitch(switches::kDisableQuic)) return false; return data_reduction_proxy::DataReductionProxyParams:: IsIncludedInQuicFieldTrial(); } bool IOThread::ShouldEnableQuicForProxies(const base::CommandLine& command_line, base::StringPiece quic_trial_group) { return ShouldEnableQuic(command_line, quic_trial_group) || ShouldEnableQuicForDataReductionProxy(command_line, quic_trial_group); } https://codereview.chromium.org/903213003/diff/220001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/220001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:463: for (size_t i = 0; i < arraysize(tests); ++i) { nit: Your call but consider writing this as: for (int i = 0; i < 2; ++i) { bool enable_quic = i == 0; ... } It's a lot fewer lines. https://codereview.chromium.org/903213003/diff/220001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/903213003/diff/220001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:447: params_.enable_quic_for_proxies = false; I don't think you need this line, do you? https://codereview.chromium.org/903213003/diff/220001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/903213003/diff/220001/net/url_request/url_req... net/url_request/url_request_context_builder.h:78: bool enable_quic_for_proxies; Does any part of this CL set this member? I'm having a hard time finding it.
https://codereview.chromium.org/903213003/diff/220001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/903213003/diff/220001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1281: } On 2015/02/14 00:09:23, Ryan Hamilton wrote: > I stared at these two methods for about 10 minutes trying to figure out the > relationship between them. (I was feeling very dense). I think you can rewrite > them to be a bit more clear and to reduce duplication (of the DRPP:IIIQFT call) > as follows: > > bool IOThread::ShouldEnableQuicForDataReductionProxy( > const base::CommandLine& command_line, > base::StringPiece quic_trial_group) { > if (command_line.HasSwitch(switches::kDisableQuic)) > return false; > > return data_reduction_proxy::DataReductionProxyParams:: > IsIncludedInQuicFieldTrial(); > } > > bool IOThread::ShouldEnableQuicForProxies(const base::CommandLine& command_line, > base::StringPiece quic_trial_group) { > return ShouldEnableQuic(command_line, quic_trial_group) || > ShouldEnableQuicForDataReductionProxy(command_line, quic_trial_group); > } Done. https://codereview.chromium.org/903213003/diff/220001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/220001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:463: for (size_t i = 0; i < arraysize(tests); ++i) { On 2015/02/14 00:09:23, Ryan Hamilton wrote: > nit: Your call but consider writing this as: > > for (int i = 0; i < 2; ++i) { > bool enable_quic = i == 0; > ... > } > > It's a lot fewer lines. Good suggestion, done. https://codereview.chromium.org/903213003/diff/220001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/903213003/diff/220001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:447: params_.enable_quic_for_proxies = false; On 2015/02/14 00:09:23, Ryan Hamilton wrote: > I don't think you need this line, do you? Done. https://codereview.chromium.org/903213003/diff/220001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/903213003/diff/220001/net/url_request/url_req... net/url_request/url_request_context_builder.h:78: bool enable_quic_for_proxies; On 2015/02/14 00:09:23, Ryan Hamilton wrote: > Does any part of this CL set this member? I'm having a hard time finding it. Done.
Thanks for making these changes. I'm quite happy with the state of this CL now. I do have one more change I think you can make, but LGTM otherwise. https://codereview.chromium.org/903213003/diff/240001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/240001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:760: using_quic_proxy_ = true; Is this member read anywhere? If not, can you remove it?
New patchsets have been uploaded after l-g-t-m from rch@chromium.org
Thanks for the comment. https://codereview.chromium.org/903213003/diff/240001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/903213003/diff/240001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:760: using_quic_proxy_ = true; On 2015/02/17 02:53:55, Ryan Hamilton wrote: > Is this member read anywhere? If not, can you remove it? Done.
https://codereview.chromium.org/903213003/diff/260001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/260001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:329: command_line_origin_ = origin; It looks like GetDefaultDevOrigin() can return something that doesn't come from the command line, so I'm not sure this is correct.
https://codereview.chromium.org/903213003/diff/260001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/903213003/diff/260001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:329: command_line_origin_ = origin; On 2015/02/17 17:30:47, bengr wrote: > It looks like GetDefaultDevOrigin() can return something that doesn't come from > the command line, so I'm not sure this is correct. would override_quic_origin_ (or override_origin_) be a better name?
Moved EnableQuic back to UI thread since enabling on IO thread had no effect. ptal.
On 2015/02/17 22:48:12, tbansal1 wrote: > Moved EnableQuic back to UI thread since enabling on IO thread had no effect. > ptal. Looks like this changed got that I previously ok. Making NOT LGTM while I review. Can you explain what you mean about enabling on IO thread vs UI thread?
On 2015/02/17 23:16:40, Ryan Hamilton wrote: > On 2015/02/17 22:48:12, tbansal1 wrote: > > Moved EnableQuic back to UI thread since enabling on IO thread had no effect. > > ptal. > > Looks like this changed got that I previously ok. Making NOT LGTM while I > review. > > Can you explain what you mean about enabling on IO thread vs UI thread? Currently, DRP maintains two copies of DRP::Params. One gets initialized by IO thread, other by UI thread (We have plans to merge them, but that's for layer). IIUC, IOThread clones DRP::Params object from the one created by UIThread. In profile_impl_io_data.cc, if we enable DRP QUIC when calling DataReductionProxyIOData::Init(), it only modifies one copy of the params (because Init() happens after the clone is finished). So, we have to enable DRP QUIC in ProfileImplIOData::Handle::Init() which happens before the clone.
On 2015/02/17 23:27:12, tbansal1 wrote: > On 2015/02/17 23:16:40, Ryan Hamilton wrote: > > On 2015/02/17 22:48:12, tbansal1 wrote: > > > Moved EnableQuic back to UI thread since enabling on IO thread had no > effect. > > > ptal. > > > > Looks like this changed got that I previously ok. Making NOT LGTM while I > > review. > > > > Can you explain what you mean about enabling on IO thread vs UI thread? > > Currently, DRP maintains two copies of DRP::Params. One gets > initialized by IO thread, other by UI thread (We have plans to merge > them, but that's for layer). IIUC, IOThread clones DRP::Params object > from the one created by UIThread. > In profile_impl_io_data.cc, if we enable DRP QUIC > when calling DataReductionProxyIOData::Init(), it only modifies one > copy of the params (because Init() happens after the clone is finished). > So, we have to enable DRP QUIC in ProfileImplIOData::Handle::Init() > which happens before the clone. After discussing, it seems it is better to wait for us to wait for the other CL that merges all the different copies of DRP:Params into one unified object. That will make this CL less ugly. So, please hold on the reviews for few more days.
On 2015/02/18 17:31:40, tbansal1 wrote: > On 2015/02/17 23:27:12, tbansal1 wrote: > > On 2015/02/17 23:16:40, Ryan Hamilton wrote: > > > On 2015/02/17 22:48:12, tbansal1 wrote: > > > > Moved EnableQuic back to UI thread since enabling on IO thread had no > > effect. > > > > ptal. > > > > > > Looks like this changed got that I previously ok. Making NOT LGTM while I > > > review. > > > > > > Can you explain what you mean about enabling on IO thread vs UI thread? > > > > Currently, DRP maintains two copies of DRP::Params. One gets > > initialized by IO thread, other by UI thread (We have plans to merge > > them, but that's for layer). IIUC, IOThread clones DRP::Params object > > from the one created by UIThread. > > In profile_impl_io_data.cc, if we enable DRP QUIC > > when calling DataReductionProxyIOData::Init(), it only modifies one > > copy of the params (because Init() happens after the clone is finished). > > So, we have to enable DRP QUIC in ProfileImplIOData::Handle::Init() > > which happens before the clone. > > After discussing, it seems it is better to wait for us to wait for the > other CL that merges all the different copies of DRP:Params into one > unified object. That will make this CL less ugly. So, please hold on > the reviews for few more days. Thanks!
On 2015/02/18 17:31:54, tbansal1 wrote: > On 2015/02/18 17:31:40, tbansal1 wrote: > > On 2015/02/17 23:27:12, tbansal1 wrote: > > > On 2015/02/17 23:16:40, Ryan Hamilton wrote: > > > > On 2015/02/17 22:48:12, tbansal1 wrote: > > > > > Moved EnableQuic back to UI thread since enabling on IO thread had no > > > effect. > > > > > ptal. > > > > > > > > Looks like this changed got that I previously ok. Making NOT LGTM while I > > > > review. > > > > > > > > Can you explain what you mean about enabling on IO thread vs UI thread? > > > > > > Currently, DRP maintains two copies of DRP::Params. One gets > > > initialized by IO thread, other by UI thread (We have plans to merge > > > them, but that's for layer). IIUC, IOThread clones DRP::Params object > > > from the one created by UIThread. > > > In profile_impl_io_data.cc, if we enable DRP QUIC > > > when calling DataReductionProxyIOData::Init(), it only modifies one > > > copy of the params (because Init() happens after the clone is finished). > > > So, we have to enable DRP QUIC in ProfileImplIOData::Handle::Init() > > > which happens before the clone. > > > > After discussing, it seems it is better to wait for us to wait for the > > other CL that merges all the different copies of DRP:Params into one > > unified object. That will make this CL less ugly. So, please hold on > > the reviews for few more days. > > Thanks! Ah, that makes sense. Ok, ping me when this is ready for review again.
ptal.
lgtm Thanks for adding the TODO. I look forward to it being done :>
data_reduction_proxy lgtm, with nit. https://codereview.chromium.org/903213003/diff/320001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/903213003/diff/320001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:484: if (enable_quic){ add a space between ) and {
New patchsets have been uploaded after l-g-t-m from rch@chromium.org,bengr@chromium.org
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/903213003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)
tbansal@chromium.org changed reviewers: + sgurun@chromium.org
sgurun@chromium.org: Please review changes in android_webview/browser/*
https://codereview.chromium.org/903213003/diff/360001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/903213003/diff/360001/android_webview/browser... android_webview/browser/aw_browser_context.cc:284: false); Please add a comment explaining QUIC is disabled for webview. It is very easy to miss what false stands for.
Thanks for the comment. ptal.
On 2015/02/20 01:50:55, tbansal1 wrote: > Thanks for the comment. ptal. lgtm
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/903213003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel 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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/903213003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg 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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/903213003/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/ed0aeccb2df07b46ca69d7328d9fab6d5aae5f8e Cr-Commit-Position: refs/heads/master@{#317232} |
