Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(217)

Issue 2921503002: Add force_empty_page proxy experiment flag and simplify ShouldAccept's (Closed)

Created:
3 years, 6 months ago by dougarnett
Modified:
3 years, 6 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

Add force_empty_page proxy experiment flag and simplify ShouldAccept's Client will now send Chrome-Proxy exp=force_empty_page if LoFi is enabled via flags but Lite Pages are not enabled flags. This will allow flag-savvy users to exercise LoFi instead of Lite Page previews for testing the LoFi behavior. This experiment will be supported with the new CPAT protocol changes. In conjunction, the logic for whether to inform the proxy server that previews are accepted (via CPAT header) is substantially simplified for the new protocol path since the server will be making the decision (including whether copyright restriction applies or not). BUG=701802 Review-Url: https://codereview.chromium.org/2921503002 Cr-Commit-Position: refs/heads/master@{#479034} Committed: https://chromium.googlesource.com/chromium/src/+/4ddaa6f836d00180fc6302b25863d04347d8f274

Patch Set 1 #

Patch Set 2 : Added testLoFiForcedExperiment integration test #

Total comments: 14

Patch Set 3 : Some update per comments. #

Patch Set 4 : Collapsed ShouldAccept* methods into single ShouldAcceptServerPreview method #

Total comments: 21

Patch Set 5 : Some updated naming per bengr feedback #

Total comments: 8

Patch Set 6 : Reverted the flag accesor names (with TODO to revisit with cleanup work) #

Messages

Total messages: 43 (30 generated)
dougarnett
Here is my take on simplifying the ShouldAccept* methods plus the force_empty_image directive addition.
3 years, 6 months ago (2017-05-31 23:37:56 UTC) #8
megjablon
https://codereview.chromium.org/2921503002/diff/20001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (left): https://codereview.chromium.org/2921503002/diff/20001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#oldcode1092 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1092: if (params::IsLoFiSlowConnectionsOnlyViaFlags() || Making a note: as discussed offline ...
3 years, 6 months ago (2017-06-01 21:57:14 UTC) #11
dougarnett
https://codereview.chromium.org/2921503002/diff/20001/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/2921503002/diff/20001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1102 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1102: // Lite Pages depends on at least Lo-Fi being ...
3 years, 6 months ago (2017-06-01 23:15:13 UTC) #14
dougarnett
https://codereview.chromium.org/2921503002/diff/20001/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/2921503002/diff/20001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1102 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1102: // Lite Pages depends on at least Lo-Fi being ...
3 years, 6 months ago (2017-06-02 23:02:15 UTC) #19
dougarnett
Ben, please take a look at this simplification of consulting very few flags not wrt ...
3 years, 6 months ago (2017-06-02 23:06:17 UTC) #21
bengr
https://codereview.chromium.org/2921503002/diff/60001/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/2921503002/diff/60001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1073 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1073: const net::URLRequest& request, Do we need the whole request? ...
3 years, 6 months ago (2017-06-08 18:53:00 UTC) #24
dougarnett
https://codereview.chromium.org/2921503002/diff/20001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (left): https://codereview.chromium.org/2921503002/diff/20001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#oldcode1092 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1092: if (params::IsLoFiSlowConnectionsOnlyViaFlags() || On 2017/06/01 21:57:14, megjablon wrote: > ...
3 years, 6 months ago (2017-06-08 22:18:13 UTC) #27
bengr
lgtm https://codereview.chromium.org/2921503002/diff/60001/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/2921503002/diff/60001/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc#newcode1073 components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1073: const net::URLRequest& request, On 2017/06/08 22:18:12, dougarnett wrote: ...
3 years, 6 months ago (2017-06-09 23:08:37 UTC) #30
megjablon
https://codereview.chromium.org/2921503002/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc#newcode119 components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:119: experiments_.push_back(chrome_proxy_experiment_force_lite_page()); The server still checks that the cpat:lite-page header ...
3 years, 6 months ago (2017-06-12 23:02:28 UTC) #31
dougarnett
https://codereview.chromium.org/2921503002/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/80001/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc#newcode119 components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:119: experiments_.push_back(chrome_proxy_experiment_force_lite_page()); On 2017/06/12 23:02:28, megjablon wrote: > The server ...
3 years, 6 months ago (2017-06-13 00:07:03 UTC) #34
megjablon
lgtm
3 years, 6 months ago (2017-06-13 00:28:36 UTC) #35
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/2921503002/100001
3 years, 6 months ago (2017-06-13 15:55:33 UTC) #40
commit-bot: I haz the power
3 years, 6 months ago (2017-06-13 16:01:40 UTC) #43
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4ddaa6f836d00180fc6302b25863...

Powered by Google App Engine
This is Rietveld 408576698