Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2889993004: New CPAT support in DataReductionProxyConfig guarded by feature flag. (Closed)

Created:
8 months, 1 week ago by dougarnett
Modified:
8 months ago
Reviewers:
bengr, megjablon
CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

New CPAT support in DataReductionProxyConfig guarded by feature flag. Defines new feature "DataReductionProxyDecidesTransform" and uses it to select new behavior for ShouldEnableLoFi() and ShouldEnableLitePages that does not check the effective connection type. This is step 1 of 4 in adding new CPAT protocol support where the proxy server is meant to drive optimizations. These are the expected steps: 1. Add support in DataReductionProxyConfig (behind flag). 2. Add support in ContentLoFiDecider (behind same flag). 3. Add force-lite-page experiment support. 4. Add support for transforms directive from the server. [Also, may add about-flags support for this new feature once the feature is code complete.] Later release (once new CPAT stable): 5. Clean up old path code. Note that this CL does not check for client driven fallback nor deal with Control group setting and later clearing for the new feature flag path. BUG=701802 Review-Url: https://codereview.chromium.org/2889993004 Cr-Commit-Position: refs/heads/master@{#474808} Committed: https://chromium.googlesource.com/chromium/src/+/cbda5f6b2b2d76236388e663c3439ef91c59ec0a

Patch Set 1 #

Total comments: 20

Patch Set 2 : Updated per comment feedback #

Patch Set 3 : Factored blacklist and lofi_off_ check (and comment verbage) into helper #

Total comments: 2

Patch Set 4 : Improved comment wording for previews_decider arg #

Patch Set 5 : Added const-ness to PreviewDecider arg #

Total comments: 20

Patch Set 6 : Updates per Megan's comments #

Total comments: 24

Patch Set 7 : Updates per Ben's comments #

Patch Set 8 : Update legacy ShouldEnableLoFiInternal to return false in LitePages enabled #

Patch Set 9 : Updates new code path to accept LoFi when LitePages enabled (to allow fallback) #

Messages

Total messages: 54 (34 generated)
dougarnett
New approach on supporting the new CPAT protocol to land in a few increments with ...
8 months, 1 week ago (2017-05-17 23:37:29 UTC) #4
dougarnett
Ping
8 months ago (2017-05-19 15:44:18 UTC) #7
bengr
On 2017/05/19 15:44:18, dougarnett wrote: > Ping The steps seem reasonable. How much of the ...
8 months ago (2017-05-19 18:30:41 UTC) #8
bengr
https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1018 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1018: // Make sure request is not locally blacklisted. These ...
8 months ago (2017-05-19 20:20:47 UTC) #9
dougarnett
https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1018 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1018: // Make sure request is not locally blacklisted. On ...
8 months ago (2017-05-19 21:43:58 UTC) #12
dougarnett
https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1018 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1018: // Make sure request is not locally blacklisted. On ...
8 months ago (2017-05-19 22:29:58 UTC) #15
dougarnett
https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h#newcode308 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:308: // |previews_decider| is a non-null object that determines eligibility ...
8 months ago (2017-05-19 22:41:22 UTC) #18
bengr
https://codereview.chromium.org/2889993004/diff/40001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/40001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1051 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1051: previews::PreviewsDecider* previews_decider, Why is this not a const previews::PreviewsDecider& ...
8 months ago (2017-05-19 22:44:53 UTC) #19
dougarnett
https://codereview.chromium.org/2889993004/diff/40001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/40001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1051 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1051: previews::PreviewsDecider* previews_decider, On 2017/05/19 22:44:53, bengr wrote: > Why ...
8 months ago (2017-05-19 23:48:31 UTC) #22
megjablon
https://codereview.chromium.org/2889993004/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1013 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1013: features::kDataReductionProxyDecidesTransform)) Can this check just be moved inside the ...
8 months ago (2017-05-22 23:58:58 UTC) #25
dougarnett
https://codereview.chromium.org/2889993004/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1013 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1013: features::kDataReductionProxyDecidesTransform)) On 2017/05/22 23:58:57, megjablon wrote: > Can this ...
8 months ago (2017-05-23 16:28:45 UTC) #28
bengr
https://codereview.chromium.org/2889993004/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1014 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1014: return ShouldAcceptServerLoFi(request, previews_decider); Add curly braces. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1037 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1037: return ...
8 months ago (2017-05-23 17:51:25 UTC) #29
dougarnett
https://codereview.chromium.org/2889993004/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1014 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1014: return ShouldAcceptServerLoFi(request, previews_decider); On 2017/05/23 17:51:24, bengr wrote: > ...
8 months ago (2017-05-23 18:27:44 UTC) #32
dougarnett
Megan, per our discussion I made ShouldEnableLoFiInternal return false now if LitePages are enabled (so ...
8 months ago (2017-05-23 21:07:03 UTC) #37
dougarnett
Ping - any more comments on this one?
8 months ago (2017-05-24 19:55:34 UTC) #44
bengr
lgtm
8 months ago (2017-05-25 16:50:02 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2889993004/160001
8 months ago (2017-05-25 16:54:34 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2889993004/160001
8 months ago (2017-05-25 19:44:35 UTC) #50
megjablon
lgtm
8 months ago (2017-05-25 19:57:18 UTC) #51
commit-bot: I haz the power
8 months ago (2017-05-25 21:23:04 UTC) #54
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/cbda5f6b2b2d76236388e663c343...

Powered by Google App Engine
This is Rietveld 408576698