DataReductionProxy proto change to use exp=force_lite_page directive

DataReductionProxy proto change to use exp=force_lite_page directive
and remove use of exp=ignore_preview blacklist directive.

This is part of new CPAT protocol change but is orthogonal to the
CPAT/CPCT changes and can land separately.

BUG=701802
Review-Url: https://codereview.chromium.org/2903453003
Cr-Commit-Position: refs/heads/master@{#474656}
Committed: https://chromium.googlesource.com/chromium/src/+/09ccc9931dc83f1e90fb5b01085cdb37201f1937

[dougarnett, 2017-05-22 23:38:35]

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

[commit-bot: I haz the power, 2017-05-22 23:39:11]

Dry run: CQ is trying da patch.

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

[dougarnett, 2017-05-22 23:41:55]

Description was changed from

==========
DataSaverProxy proto change to use exp=force_lite_page directive
and remove use of exp=ignore_preview blacklist directive.

This is part of new CPAT protocol change but is orthogonal to the
CPAT/CPCT changes and can land separately.

BUG=701802
==========

to

==========
DataReductionProxy proto change to use exp=force_lite_page directive
and remove use of exp=ignore_preview blacklist directive.

This is part of new CPAT protocol change but is orthogonal to the
CPAT/CPCT changes and can land separately.

BUG=701802
==========

[dougarnett, 2017-05-22 23:56:30]

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

[megjablon, 2017-05-23 00:19:07]

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:185: if
(!headers || !params::AreLitePagesEnabledViaFlags() ||
Should we also check !params::IsLoFiSlowConnectionsOnlyViaFlags() here?

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:102:
// preview experiment to force a lite page preview.
Should we be more descriptive about what this does? "Forcing a lite page gets a
lite page, if it can be transcoded, regardless of the server blacklist and the
network quality". Or we can keep this opaque intentionally so that we can adapt
the meaning via the server as need be.

[commit-bot: I haz the power, 2017-05-23 00:51:27]

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

[commit-bot: I haz the power, 2017-05-23 00:51:28]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2017-05-23 17:02:17]

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:185: if
(!headers || !params::AreLitePagesEnabledViaFlags() ||
On 2017/05/23 00:19:07, megjablon wrote:
> Should we also check !params::IsLoFiSlowConnectionsOnlyViaFlags() here?

Hmm, I think we would need to check an OR with at least Always-On and
Cellular-Only as well as SlowConnectionViaFlags if so. Not clear to me what we
gain there (disable for mix of LoFi field trial and LitePages flag?).

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/2903453003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:102:
// preview experiment to force a lite page preview.
On 2017/05/23 00:19:07, megjablon wrote:
> Should we be more descriptive about what this does? "Forcing a lite page gets
a
> lite page, if it can be transcoded, regardless of the server blacklist and the
> network quality". Or we can keep this opaque intentionally so that we can
adapt
> the meaning via the server as need be.

Actually had feedback from Ben for the sense of the latter (from my earlier
mondo CL).

[dougarnett, 2017-05-24 16:12:09]

Any other comments on this one?

[dougarnett, 2017-05-24 18:44:51]

Actually, talked to Ryan and need to merge this with a CL he landed - so this is
not ready yet.

[dougarnett, 2017-05-24 22:17:27]

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

[commit-bot: I haz the power, 2017-05-24 22:19:03]

Dry run: CQ is trying da patch.

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

[dougarnett, 2017-05-24 22:27:28]

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

[dougarnett, 2017-05-24 22:27:29]

Ok, now merged with Ryan's change (which means less code!)

[RyanSturm, 2017-05-24 22:31:19]

lgtm

[RyanSturm, 2017-05-24 22:33:51]

https://codereview.chromium.org/2903453003/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/2903453003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:102:
// preview experiment to force a lite page preview.
Actually, one nit: Can you change this comment less to reflect it's not the
whole directive, just the value of the "exp" directive.

[dougarnett, 2017-05-24 23:59:34]

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

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

Dry run: CQ is trying da patch.

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

[dougarnett, 2017-05-25 00:00:17]

https://codereview.chromium.org/2903453003/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/2903453003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:102:
// preview experiment to force a lite page preview.
On 2017/05/24 22:33:51, RyanSturm wrote:
> Actually, one nit: Can you change this comment less to reflect it's not the
> whole directive, just the value of the "exp" directive.

Done - also changed terminology in method name

[commit-bot: I haz the power, 2017-05-25 01:52:44]

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

[commit-bot: I haz the power, 2017-05-25 01:52:44]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2017-05-25 15:12:52]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2017-05-25 15:12:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2903453003/#ps40001
(title: "Adjusted directive terminology to experiment terminology")

[commit-bot: I haz the power, 2017-05-25 15:13:22]

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-25 15:17:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495725172165520,
"parent_rev": "73228cd43d2e9794bab5a8508c5db4b23704bc84", "commit_rev":
"09ccc9931dc83f1e90fb5b01085cdb37201f1937"}

[commit-bot: I haz the power, 2017-05-25 15:17:32]

Description was changed from

==========
DataReductionProxy proto change to use exp=force_lite_page directive
and remove use of exp=ignore_preview blacklist directive.

This is part of new CPAT protocol change but is orthogonal to the
CPAT/CPCT changes and can land separately.

BUG=701802
==========

to

==========
DataReductionProxy proto change to use exp=force_lite_page directive
and remove use of exp=ignore_preview blacklist directive.

This is part of new CPAT protocol change but is orthogonal to the
CPAT/CPCT changes and can land separately.

BUG=701802

Review-Url: https://codereview.chromium.org/2903453003
Cr-Commit-Position: refs/heads/master@{#474656}
Committed:
https://chromium.googlesource.com/chromium/src/+/09ccc9931dc83f1e90fb5b01085c...
==========

[commit-bot: I haz the power, 2017-05-25 15:17:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/09ccc9931dc83f1e90fb5b01085c...