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

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

Created:
6 months ago by dougarnett
Modified:
5 months, 3 weeks 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) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+528 lines, -51 lines) Patch
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h View 1 2 3 4 5 6 5 chunks +41 lines, -9 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc View 1 2 3 4 5 6 7 8 5 chunks +114 lines, -33 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h View 1 chunk +5 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc View 1 2 3 4 5 6 7 8 5 chunks +350 lines, -1 line 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc View 1 2 3 4 5 1 chunk +4 lines, -2 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc View 1 2 3 4 5 6 7 8 4 chunks +6 lines, -6 lines 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_features.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
Trybot results:  linux_android_rel_ng   chromium_presubmit   linux_android_rel_ng   win_chromium_rel_ng   linux_chromium_tsan_rel_ng   android_compile_dbg   linux_chromium_chromeos_rel_ng   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux   ios-simulator   android_n5x_swarming_rel   linux_chromium_rel_ng   ios-device   win_clang   android_clang_dbg_recipe   mac_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   ios-device-xcode-clang   cast_shell_android   win_chromium_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   mac_chromium_rel_ng   android_arm64_dbg_recipe   ios-simulator-xcode-clang   linux_chromium_asan_rel_ng   win_chromium_x64_rel_ng   linux_chromium_compile_dbg_ng   android_cronet   chromium_presubmit   win_chromium_rel_ng   linux_android_rel_ng   android_compile_dbg   linux_chromium_tsan_rel_ng   linux_chromium_chromeos_rel_ng   ios-simulator   chromeos_daisy_chromium_compile_only_ng   chromium_presubmit   cast_shell_linux   android_n5x_swarming_rel   ios-device   win_clang   mac_chromium_compile_dbg_ng   android_clang_dbg_recipe   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_android   ios-device-xcode-clang   linux_chromium_chromeos_ozone_rel_ng   win_chromium_compile_dbg_ng   linux_chromium_rel_ng   android_arm64_dbg_recipe   ios-simulator-xcode-clang   mac_chromium_rel_ng   win_chromium_x64_rel_ng   linux_chromium_asan_rel_ng   linux_chromium_compile_dbg_ng   android_cronet 

Messages

Total messages: 54 (34 generated)
dougarnett
New approach on supporting the new CPAT protocol to land in a few increments with ...
6 months ago (2017-05-17 23:37:29 UTC) #4
dougarnett
Ping
6 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 ...
6 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 ...
6 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 ...
6 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 ...
6 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 ...
6 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& ...
6 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 ...
6 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 ...
6 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 ...
6 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 ...
5 months, 4 weeks 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: > ...
5 months, 4 weeks 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 ...
5 months, 4 weeks ago (2017-05-23 21:07:03 UTC) #37
dougarnett
Ping - any more comments on this one?
5 months, 4 weeks ago (2017-05-24 19:55:34 UTC) #44
bengr
lgtm
5 months, 4 weeks 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
5 months, 4 weeks 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
5 months, 3 weeks ago (2017-05-25 19:44:35 UTC) #50
megjablon
lgtm
5 months, 3 weeks ago (2017-05-25 19:57:18 UTC) #51
commit-bot: I haz the power
5 months, 3 weeks 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 efc10ee0f