Add an about:flag to support alternative data saver features

Add an about:flag to support alternative data saver features

This flag allows an alternative back end Data Saver implementation to be
used by chrome. This may result in different types of previews served,
different triggering mechanisms, or other back end changes that are not
used by default.

This also fixes the CP header having multiple "exp" directives.

BUG=721113, 656195
Review-Url: https://codereview.chromium.org/2887423002
Cr-Commit-Position: refs/heads/master@{#474320}
Committed: https://chromium.googlesource.com/chromium/src/+/7e309dd054ce82c89ff193ca57f559d025ccbe4d

[RyanSturm, 2017-05-18 18:41:06]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 18:42:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 19:17:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 19:17:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[RyanSturm, 2017-05-18 20:31:23]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:33:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-05-18 20:36:27]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:38:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-05-18 21:21:19]

ryansturm@chromium.org changed reviewers:
+ buettner@chromium.org, tbansal@chromium.org

[RyanSturm, 2017-05-18 21:21:20]

tbansal, buettner: PTAL

[commit-bot: I haz the power, 2017-05-19 01:51:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 01:51:44]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-19 16:27:23]

https://codereview.chromium.org/2887423002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2887423002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1895:
{"enable-data-reduction-proxy-alternative-back-end",
I still think this should be a dropdown flag (with two options: Default and
Alternative). The flag should map to the existing switch for setting the server
experiments.

Right now, we have two different switches which do exactly the same thing.
Having a single switch would reduce the confusion. Also, see:
https://bugs.chromium.org/p/chromium/issues/detail?id=656195#c1

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(left):

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:183:
void ContentLoFiDecider::MaybeSetIgnorePreviewsBlacklistDirective(
Thanks for the cleanup. The new logic is much better.

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:106:
std::string experiments =
DCHECK(experiments_.empty());

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:129:
RegenerateRequestHeaderValue();
DCHECK |experiments_| has size <= 1.

https://codereview.chromium.org/2887423002/diff/20001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lite_page.py (right):

https://codereview.chromium.org/2887423002/diff/20001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lite_page.py:33:
'--data-reduction-proxy-experiment' not in
It seems that the test will fail if
--data-reduction-proxy-experiment
is already set?

In that case, may be just skip the test?

[RyanSturm, 2017-05-23 21:53:09]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 21:53:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-05-23 22:03:36]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 22:04:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-05-23 22:04:58]

tbansal: PTAL. There's no good way to get lite pages to use the flag correctly
from about:flags, but I did add a dropdown and set the switch directly from
about:flags

https://codereview.chromium.org/2887423002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2887423002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1895:
{"enable-data-reduction-proxy-alternative-back-end",
On 2017/05/19 16:27:23, tbansal1 wrote:
> I still think this should be a dropdown flag (with two options: Default and
> Alternative). The flag should map to the existing switch for setting the
server
> experiments.
> 
> Right now, we have two different switches which do exactly the same thing.
> Having a single switch would reduce the confusion. Also, see:
> https://bugs.chromium.org/p/chromium/issues/detail?id=656195#c1

Done.

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(left):

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:183:
void ContentLoFiDecider::MaybeSetIgnorePreviewsBlacklistDirective(
On 2017/05/19 16:27:23, tbansal1 wrote:
> Thanks for the cleanup. The new logic is much better.

Acknowledged.

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:106:
std::string experiments =
On 2017/05/19 16:27:23, tbansal1 wrote:
> DCHECK(experiments_.empty());

Done.

https://codereview.chromium.org/2887423002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:129:
RegenerateRequestHeaderValue();
On 2017/05/19 16:27:23, tbansal1 wrote:
> DCHECK |experiments_| has size <= 1.

Done.

https://codereview.chromium.org/2887423002/diff/20001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lite_page.py (right):

https://codereview.chromium.org/2887423002/diff/20001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lite_page.py:33:
'--data-reduction-proxy-experiment' not in
On 2017/05/19 16:27:23, tbansal1 wrote:
> It seems that the test will fail if
> --data-reduction-proxy-experiment
> is already set?
> 
> In that case, may be just skip the test?

All of our tested configurations will pass.

[commit-bot: I haz the power, 2017-05-23 22:36:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 22:36:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[RyanSturm, 2017-05-23 22:46:31]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 22:47:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-05-23 23:13:35]

lgtm % nits. Feel free to skip the one comment that is not related to this CL.

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:126:
RegenerateRequestHeaderValue();
nit: DCHECK_GE(1, experiments_.size());

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc
(right):

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:21:
// Gets the Chrome-Proxy "exp" directive value used by data reduction proxy to
rm "Gets" and update comment.

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h
(right):

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h:11:
// All switches in alphabetical order. The switches should be documented
nit: rm comment.

[RyanSturm, 2017-05-23 23:18:11]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[RyanSturm, 2017-05-23 23:18:11]

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:126:
RegenerateRequestHeaderValue();
On 2017/05/23 23:13:34, tbansal1 wrote:
> nit: DCHECK_GE(1, experiments_.size());

Acknowledged.

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc
(right):

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:21:
// Gets the Chrome-Proxy "exp" directive value used by data reduction proxy to
On 2017/05/23 23:13:34, tbansal1 wrote:
> rm "Gets" and update comment.

Done.

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h
(right):

https://codereview.chromium.org/2887423002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h:11:
// All switches in alphabetical order. The switches should be documented
On 2017/05/23 23:13:35, tbansal1 wrote:
> nit: rm comment.

Acknowledged.

[commit-bot: I haz the power, 2017-05-23 23:19:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[buettner, 2017-05-23 23:32:13]

https://codereview.chromium.org/2887423002/diff/100001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2887423002/diff/100001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:1166: "Use alternative server
configuration";
Just curious, but why are there three descriptions that say essentially the same
thing using slightly different words?

[commit-bot: I haz the power, 2017-05-24 01:04:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 01:04:28]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-05-24 16:16:03]

https://codereview.chromium.org/2887423002/diff/100001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2887423002/diff/100001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:1166: "Use alternative server
configuration";
On 2017/05/23 23:32:13, buettner wrote:
> Just curious, but why are there three descriptions that say essentially the
same
> thing using slightly different words?
> 

1) what the text in the drop down says
2) What the bold name of the flag is
3) what the text below the name says

I am completely open to suggestions (for instance changing the drop down text to
just "alternative" to match the exp header" or changing the name to something
shorter "Alternative Data Saver back end.")

[buettner, 2017-05-24 16:22:33]

On 2017/05/24 16:16:03, RyanSturm wrote:
>
https://codereview.chromium.org/2887423002/diff/100001/chrome/browser/flag_de...
> File chrome/browser/flag_descriptions.cc (right):
> 
>
https://codereview.chromium.org/2887423002/diff/100001/chrome/browser/flag_de...
> chrome/browser/flag_descriptions.cc:1166: "Use alternative server
> configuration";
> On 2017/05/23 23:32:13, buettner wrote:
> > Just curious, but why are there three descriptions that say essentially the
> same
> > thing using slightly different words?
> > 
> 
> 1) what the text in the drop down says
> 2) What the bold name of the flag is
> 3) what the text below the name says
> 
> I am completely open to suggestions (for instance changing the drop down text
to
> just "alternative" to match the exp header" or changing the name to something
> shorter "Alternative Data Saver back end.")

lgtm

[RyanSturm, 2017-05-24 16:23:46]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-05-24 16:23:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2887423002/#ps100001
(title: "tbansal nits")

[commit-bot: I haz the power, 2017-05-24 16:24:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 16:29:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495643026742660,
"parent_rev": "6e5e3ee024bee44416943dffdd4f60084357728f", "commit_rev":
"7e309dd054ce82c89ff193ca57f559d025ccbe4d"}

[commit-bot: I haz the power, 2017-05-24 16:30:08]

Description was changed from

==========
Add an about:flag to support alternative data saver features

This flag allows an alternative back end Data Saver implementation to be
used by chrome. This may result in different types of previews served,
different triggering mechanisms, or other back end changes that are not
used by default.

This also fixes the CP header having multiple "exp" directives.

BUG=721113,656195
==========

to

==========
Add an about:flag to support alternative data saver features

This flag allows an alternative back end Data Saver implementation to be
used by chrome. This may result in different types of previews served,
different triggering mechanisms, or other back end changes that are not
used by default.

This also fixes the CP header having multiple "exp" directives.

BUG=721113,656195

Review-Url: https://codereview.chromium.org/2887423002
Cr-Commit-Position: refs/heads/master@{#474320}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e309dd054ce82c89ff193ca57f5...
==========

[commit-bot: I haz the power, 2017-05-24 16:30:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7e309dd054ce82c89ff193ca57f5...

[dougarnett, 2017-05-26 23:24:42]

dougarnett@chromium.org changed reviewers:
+ dougarnett@chromium.org

[dougarnett, 2017-05-26 23:24:43]

https://codereview.chromium.org/2887423002/diff/100001/tools/chrome_proxy/web...
File tools/chrome_proxy/webdriver/lite_page.py (right):

https://codereview.chromium.org/2887423002/diff/100001/tools/chrome_proxy/web...
tools/chrome_proxy/webdriver/lite_page.py:16: # If it was attempted to run with
another experiment, skip this test.
This comment no longer belongs here as the related code got below to an inner
scope.

https://codereview.chromium.org/2887423002/diff/100001/tools/chrome_proxy/web...
tools/chrome_proxy/webdriver/lite_page.py:46: # If it was attempted to run with
another experiment, skip this test.
ditto