Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2424973002:
Only match host port pair when determining if a proxy is a data reduction proxy (Closed)
DescriptionOnly match host port pair when determining if a proxy is a data reduction proxy
This ensures that a valid data reduction QUIC proxy is
actually determined as a data reduction proxy.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet
BUG=656727
Committed: https://crrev.com/663c27f0b48b3f49725eb2095f716db80d49fad5
Cr-Commit-Position: refs/heads/master@{#425764}
Patch Set 1 : ps #
Total comments: 4
Messages
Total messages: 23 (17 generated)
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix BUG= ========== to ========== Only match host port pair when determining if a proxy is a data reduction proxy BUG= ==========
Description was changed from ========== Only match host port pair when determining if a proxy is a data reduction proxy BUG= ========== to ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. BUG= ==========
Description was changed from ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. BUG= ========== to ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. BUG=656727 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. BUG=656727 ========== to ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=656727 ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. thanks.
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/v2/patch-status/codereview.chromium.or...
lgtm, I left comments that don't need to be addressed in this CL. https://codereview.chromium.org/2424973002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2424973002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:355: config_values_->proxies_for_http(); proxies_for_http() frustrates me. Either we have a seperate lists of ProxyServers for different schemes or we have one list of HostPortPairs. This is fine for the merge, but can you open a bug on this, so it gets fixed at some point. https://codereview.chromium.org/2424973002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:360: [&host_port_pair](const net::ProxyServer& proxy) { nit: I don't care if you change this or not, but since the scope of this lambda means it will be short lived, you don't strictly need to capture host_port_pair (instead you can just [&]). the style guide is ambiguous in this area as which is preferred for short-lvied lambdas. https://google.github.io/styleguide/cppguide.html#Lambda_expressions This is a total nit, and I don't care if you fix it; just a talking point.
https://codereview.chromium.org/2424973002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2424973002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:355: config_values_->proxies_for_http(); On 2016/10/17 18:57:22, Ryan Sturm wrote: > proxies_for_http() frustrates me. Either we have a seperate lists of > ProxyServers for different schemes or we have one list of HostPortPairs. > > This is fine for the merge, but can you open a bug on this, so it gets fixed at > some point. We used to have proxies_for_https() in the past for CONNECT jobs. Later, we removed the code. We are not yet 100% confident that we will not bring that code back. So, it might be too premature to refactor this to a different name. https://codereview.chromium.org/2424973002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:360: [&host_port_pair](const net::ProxyServer& proxy) { On 2016/10/17 18:57:22, Ryan Sturm wrote: > nit: I don't care if you change this or not, but since the scope of this lambda > means it will be short lived, you don't strictly need to capture host_port_pair > (instead you can just [&]). the style guide is ambiguous in this area as which > is preferred for short-lvied lambdas. > > https://google.github.io/styleguide/cppguide.html#Lambda_expressions > > This is a total nit, and I don't care if you fix it; just a talking point. Acknowledged.
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/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=656727 ========== to ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=656727 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=656727 ========== to ========== Only match host port pair when determining if a proxy is a data reduction proxy This ensures that a valid data reduction QUIC proxy is actually determined as a data reduction proxy. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=656727 Committed: https://crrev.com/663c27f0b48b3f49725eb2095f716db80d49fad5 Cr-Commit-Position: refs/heads/master@{#425764} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/663c27f0b48b3f49725eb2095f716db80d49fad5 Cr-Commit-Position: refs/heads/master@{#425764} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
