3 years, 9 months ago
(2017-03-27 06:35:48 UTC)
#5
Patchset #1 (id:1) has been deleted
tbansal1
Description was changed from ========== Bypass DRP on Redirect cycle BUG= ========== to ========== Bypass ...
3 years, 9 months ago
(2017-03-27 06:36:46 UTC)
#6
Description was changed from
==========
Bypass DRP on Redirect cycle
BUG=
==========
to
==========
Bypass DRP on redirect cycle
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=
==========
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 9 months ago
(2017-03-27 07:29:25 UTC)
#7
3 years, 9 months ago
(2017-03-27 07:29:26 UTC)
#8
Dry run: This issue passed the CQ dry run.
tbansal1
Description was changed from ========== Bypass DRP on redirect cycle If a redirect cycle is ...
3 years, 9 months ago
(2017-03-27 16:54:09 UTC)
#9
Description was changed from
==========
Bypass DRP on redirect cycle
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=
==========
to
==========
Bypass DRP if a redirect cycle is detected
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=664586
==========
tbansal1
Patchset #2 (id:40001) has been deleted
3 years, 9 months ago
(2017-03-27 17:05:39 UTC)
#10
Patchset #2 (id:40001) has been deleted
tbansal1
Patchset #1 (id:20001) has been deleted
3 years, 9 months ago
(2017-03-27 17:22:44 UTC)
#11
Patchset #1 (id:20001) has been deleted
tbansal1
Patchset #1 (id:60001) has been deleted
3 years, 9 months ago
(2017-03-27 17:23:05 UTC)
#12
Patchset #1 (id:60001) has been deleted
tbansal1
Patchset #1 (id:70001) has been deleted
3 years, 9 months ago
(2017-03-27 17:23:11 UTC)
#13
Patchset #1 (id:70001) has been deleted
tbansal1
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-27 17:23:21 UTC)
#14
3 years, 9 months ago
(2017-03-27 19:01:20 UTC)
#19
Dry run: This issue passed the CQ dry run.
tbansal1
Description was changed from ========== Bypass DRP if a redirect cycle is detected If a ...
3 years, 9 months ago
(2017-03-27 21:36:17 UTC)
#20
Description was changed from
==========
Bypass DRP if a redirect cycle is detected
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=664586
==========
to
==========
Bypass DRP if a redirect cycle is detected
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=664586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========
RyanSturm
https://codereview.chromium.org/2777823002/diff/90001/components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc (right): https://codereview.chromium.org/2777823002/diff/90001/components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc#newcode340 components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:340: std::unique_ptr<net::URLRequest> CreateAndExecuteURLRedirectCycleRequest() { This is a really good test. ...
3 years, 8 months ago
(2017-03-28 18:03:19 UTC)
#21
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
(right):
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:340:
std::unique_ptr<net::URLRequest> CreateAndExecuteURLRedirectCycleRequest() {
This is a really good test.
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
(right):
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:123:
if (last_entry_number_occurences >= 2)
This looks confusing with the >=2 condition. Since you are O(n) of the chain
anyway, maybe you should just do something like:
vector<GURL> everything_but_the_back(url_chain.front(), url_chain.back());
for (const auto& url : everything_but_the_back) {
if (url == last_entry)
return true;
}
return false;
The most efficient option might be (it doesn't have to walk the whole chain or
copy the chain):
auto last_entry = std::prev(url_chain.end());
for (auto url : url_chain) {
if (url == last_entry)
continue;
if (*url == *last_entry)
return true;
}
return false;
You could also re-write the above to use a for loop with the termination
condition of iter != std::prev(vector.end())
My syntax might be a little off.
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:312:
bool has_via_header = HasDataReductionProxyViaHeader(&headers, nullptr);
Any chance you want to change HasDataReductionProxyViaHeader to accept a const
ref to headers?
tbansal1
Patchset #2 (id:110001) has been deleted
3 years, 8 months ago
(2017-03-28 18:26:14 UTC)
#22
3 years, 8 months ago
(2017-03-28 19:59:49 UTC)
#23
ryansturm: ptal. Thanks.
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
(right):
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:340:
std::unique_ptr<net::URLRequest> CreateAndExecuteURLRedirectCycleRequest() {
On 2017/03/28 18:03:19, Ryan Sturm wrote:
> This is a really good test.
Acknowledged.
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
(right):
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:123:
if (last_entry_number_occurences >= 2)
On 2017/03/28 18:03:19, Ryan Sturm wrote:
> This looks confusing with the >=2 condition. Since you are O(n) of the chain
> anyway, maybe you should just do something like:
>
> vector<GURL> everything_but_the_back(url_chain.front(), url_chain.back());
> for (const auto& url : everything_but_the_back) {
> if (url == last_entry)
> return true;
> }
> return false;
>
>
> The most efficient option might be (it doesn't have to walk the whole chain or
> copy the chain):
>
> auto last_entry = std::prev(url_chain.end());
> for (auto url : url_chain) {
> if (url == last_entry)
> continue;
> if (*url == *last_entry)
> return true;
> }
> return false;
>
> You could also re-write the above to use a for loop with the termination
> condition of iter != std::prev(vector.end())
>
>
> My syntax might be a little off.
Done.
https://codereview.chromium.org/2777823002/diff/90001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:312:
bool has_via_header = HasDataReductionProxyViaHeader(&headers, nullptr);
On 2017/03/28 18:03:19, Ryan Sturm wrote:
> Any chance you want to change HasDataReductionProxyViaHeader to accept a const
> ref to headers?
I was planning to do it in a separate CL, but I have included those changes in
this CL itself. PTAL. Thanks.
tbansal1
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
3 years, 8 months ago
(2017-03-28 19:59:53 UTC)
#24
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/383333)
3 years, 8 months ago
(2017-03-28 20:10:02 UTC)
#27
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/398010)
3 years, 8 months ago
(2017-03-29 22:00:06 UTC)
#36
CQ is committing da patch. Bot data: {"patchset_id": 130001, "attempt_start_ts": 1490838239191340, "parent_rev": "f526b12b1f6e9d442806782bdd49dce8bceb3580", "commit_rev": "412c3e37d5e70522d3ff5a0961c58a1f624d3ef7"}
3 years, 8 months ago
(2017-03-30 04:00:04 UTC)
#42
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1490838239191340,
"parent_rev": "f526b12b1f6e9d442806782bdd49dce8bceb3580", "commit_rev":
"412c3e37d5e70522d3ff5a0961c58a1f624d3ef7"}
commit-bot: I haz the power
Description was changed from ========== Bypass DRP if a redirect cycle is detected If a ...
3 years, 8 months ago
(2017-03-30 04:01:12 UTC)
#43
Message was sent while issue was closed.
Description was changed from
==========
Bypass DRP if a redirect cycle is detected
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=664586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========
to
==========
Bypass DRP if a redirect cycle is detected
If a redirect cycle is detected, Data Reduction Proxy (DRP) is bypassed
for the current request.
BUG=664586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2777823002
Cr-Commit-Position: refs/heads/master@{#460656}
Committed:
https://chromium.googlesource.com/chromium/src/+/412c3e37d5e70522d3ff5a0961c5...
==========
commit-bot: I haz the power
Committed patchset #2 (id:130001) as https://chromium.googlesource.com/chromium/src/+/412c3e37d5e70522d3ff5a0961c58a1f624d3ef7
3 years, 8 months ago
(2017-03-30 04:01:13 UTC)
#44
Issue 2777823002: Bypass DRP if a redirect cycle is detected
(Closed)
Created 3 years, 9 months ago by tbansal1
Modified 3 years, 8 months ago
Reviewers: RyanSturm, rkaplow
Base URL:
Comments: 6