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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by dougarnett
Modified:
9 hours, 55 minutes 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 
Commit queue not available (can’t edit this change).

Messages

Total messages: 54 (34 generated)
dougarnett
New approach on supporting the new CPAT protocol to land in a few increments with ...
1 week, 1 day ago (2017-05-17 23:37:29 UTC) #4
dougarnett
Ping
6 days, 15 hours 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 days, 12 hours 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 days, 10 hours 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 days, 9 hours 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 days, 8 hours 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 days, 8 hours 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 days, 8 hours 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 days, 7 hours 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 ...
3 days, 7 hours 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 ...
2 days, 14 hours 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 ...
2 days, 13 hours 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: > ...
2 days, 12 hours 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 ...
2 days, 10 hours ago (2017-05-23 21:07:03 UTC) #37
dougarnett
Ping - any more comments on this one?
1 day, 11 hours ago (2017-05-24 19:55:34 UTC) #44
bengr
lgtm
14 hours, 28 minutes 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
14 hours, 24 minutes 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
11 hours, 34 minutes ago (2017-05-25 19:44:35 UTC) #50
megjablon
lgtm
11 hours, 21 minutes ago (2017-05-25 19:57:18 UTC) #51
commit-bot: I haz the power
9 hours, 55 minutes 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06