Update CPAT protocol to send lite-page transform acceptance with ect

Update CPAT protocol to send lite-page transform acceptance with ect
header without client limiting by effective connection. In particular,
this removes NQE client checks for LitePage previews and removes
";if_heavy" header qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Also adds some ECT header integration test coverage in lite_page.py.

Does not yet add support for empty-image accept with ect.
We first need the server support for alt-transforms directive
to stop checking ect on client for LoFi. 

 
BUG=701802, 691135
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[dougarnett, 2017-04-05 22:50:29]

Description was changed from

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header.
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

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

to

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header.
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougarnett, 2017-04-05 22:50:38]

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

[commit-bot: I haz the power, 2017-04-05 22:50:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-04-05 22:58:43]

Description was changed from

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header.
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header.
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougarnett, 2017-04-05 22:59:05]

Description was changed from

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header.
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header).
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

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

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

[commit-bot: I haz the power, 2017-04-05 23:27:20]

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

[dougarnett, 2017-04-20 18:54:09]

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

[commit-bot: I haz the power, 2017-04-20 18:54:35]

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-04-20 19:41:29]

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

[commit-bot: I haz the power, 2017-04-20 19:41:30]

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

[dougarnett, 2017-04-20 21:45:06]

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

[commit-bot: I haz the power, 2017-04-20 21:45:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-04-20 22:22:52]

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

[commit-bot: I haz the power, 2017-04-20 22:23:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-04-20 22:24:42]

Description was changed from

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header).
Adds initial support for server subresource directive:
alt-preview=empty-image.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header).

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for alt-transforms directive nor for
adding the cr-restrict header. Those will be in subsequent CL
(when server supports them).
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-04-20 22:26:04]

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

[commit-bot: I haz the power, 2017-04-20 22:26:05]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dougarnett, 2017-04-20 22:28:08]

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

[dougarnett, 2017-04-20 22:28:09]

Back to updating CPAT for M60 now. This CL is a bit
scaled back from previous one to not yet try to handle
alt-transforms (need server support before doing that).

[dougarnett, 2017-04-21 21:14:42]

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

[commit-bot: I haz the power, 2017-04-21 21:15:09]

Dry run: CQ is trying da patch.

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

[dougarnett, 2017-04-21 21:22:28]

Description was changed from

==========
Update CPAT protocol (removes NQE client checks and ";if_heavy" header).

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for alt-transforms directive nor for
adding the cr-restrict header. Those will be in subsequent CL
(when server supports them).
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol to send lite-page transform acceptance with ect header
without client limiting by effective conneciton. In particular, this removes NQE
client checks for LitePage previews and removes ";if_heavy" header
qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect. We first need the
server support for alt-transforms directive to stop checking ect on client for
LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougarnett, 2017-04-21 21:37:22]

Updated so that now LoFi will check NQE locally whereas LitePage now will not.
Seems like a good split. When server provides the alt-transforms directive, then
we can stop checking NQE on client.

[commit-bot: I haz the power, 2017-04-21 21:50:45]

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

[commit-bot: I haz the power, 2017-04-21 21:50:46]

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

[dougarnett, 2017-04-24 17:48:21]

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

[commit-bot: I haz the power, 2017-04-24 17:49:15]

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-04-24 19:40:53]

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

[commit-bot: I haz the power, 2017-04-24 19:40:55]

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

[dougarnett, 2017-04-25 20:27:39]

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

[commit-bot: I haz the power, 2017-04-25 20:28:30]

Dry run: CQ is trying da patch.

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

[dougarnett, 2017-04-25 20:29:40]

Ping Megan. Can you take a look this week?

[dougarnett, 2017-04-25 21:04:16]

Description was changed from

==========
Update CPAT protocol to send lite-page transform acceptance with ect header
without client limiting by effective conneciton. In particular, this removes NQE
client checks for LitePage previews and removes ";if_heavy" header
qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect. We first need the
server support for alt-transforms directive to stop checking ect on client for
LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol to send lite-page transform acceptance with ect header
without client limiting by effective conneciton. In particular, this removes NQE
client checks for LitePage previews and removes ";if_heavy" header
qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect. We first need the
server support for alt-transforms directive to stop checking ect on client for
LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougarnett, 2017-04-25 21:05:08]

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

[dougarnett, 2017-04-25 21:05:08]

Adding Ben

[commit-bot: I haz the power, 2017-04-25 22:12:34]

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

[commit-bot: I haz the power, 2017-04-25 22:12:35]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-04-25 23:10:37]

A quick pass. I haven't reviewed tests yet. Also, can you update the
description? The first line should be the same as the subject and the lines
shouldn't be longer than 80 characters.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:70: //
has previews turned off.
s/if the page load/if the request

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:89: }
else if (previews_state & content::SERVER_LOFI_ON) {
I'd keep resource_type_supports_empty_image and use

} else if ((previews_state & content::SERVER_LOFI_ON) &&
           resource_type_supports_empty_image) {

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:157: if
(!params::AreLitePagesEnabledViaFlags() &&
Why do we care about cellular only here?

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:158:
!params::IsLoFiCellularOnlyViaFlags())
Use brackets

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(left):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:873:
DCHECK((params::IsIncludedInLoFiEnabledFieldTrial() ||
Keep these DCHECKS? Is this cl just a intermediate state and alt-transform cl
will be soon to follow and make it into M60?

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:637:
if (!params::IsLitePageFallbackEnabled())
Once we have alt-transform should we even have IsLitePageFallbackEnabled()? My
understanding is this new protocol was to support lite page fallback via the
server, and my field trial/client impl was just so we could get the fallback in
sooner. If so, add a comment here to remove the check.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:195:
// proxy server asking for them (via alt-transforms directive).
I thought this didn't support alt-transforms yet and still checks ECT?

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(left):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:100:
// mode only on slow connections.
Why this change?

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:28: // header if
needed. If a slow page preview is triggered, adds "lite-page" or
s/If a slow page preview is/If a preview

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:54: // the page not
being heavy).
(in spite of a good network connection, the page being blacklisted, or the page
not being heavy)?

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:59: // true if Lo-Fi
is enabled for |request| or Chrome session is in Lo-Fi
This should still be 'and'

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:61: virtual bool
ShouldRecordLoFiUMA(const net::URLRequest& request) const = 0;
Rename this to ShouldRecordPreviewsUMA? Since it does return true for Lite Page
users. Lite Pages use to be a subset of Lo-Fi, but now that we're starting to
look at them as mostly separate previews types we should change that language
wherever possible.

[dougarnett, 2017-04-26 16:34:43]

Description was changed from

==========
Update CPAT protocol to send lite-page transform acceptance with ect header
without client limiting by effective conneciton. In particular, this removes NQE
client checks for LitePage previews and removes ";if_heavy" header
qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect. We first need the
server support for alt-transforms directive to stop checking ect on client for
LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol to send lite-page transform acceptance with ect
header without client limiting by effective conneciton. In particular,
this removes NQE client checks for LitePage previews and removes
";if_heavy" header qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect.
We first need the server support for alt-transforms directive
to stop checking ect on client for LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougarnett, 2017-04-26 18:09:06]

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

[commit-bot: I haz the power, 2017-04-26 18:09:34]

Dry run: CQ is trying da patch.

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

[dougarnett, 2017-04-26 18:19:33]

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

[commit-bot: I haz the power, 2017-04-26 18:20: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-04-26 19:50:49]

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:70: //
has previews turned off.
On 2017/04/25 23:10:36, megjablon wrote:
> s/if the page load/if the request

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:89: }
else if (previews_state & content::SERVER_LOFI_ON) {
On 2017/04/25 23:10:36, megjablon wrote:
> I'd keep resource_type_supports_empty_image and use
> 
> } else if ((previews_state & content::SERVER_LOFI_ON) &&
>            resource_type_supports_empty_image) {

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:157: if
(!params::AreLitePagesEnabledViaFlags() &&
On 2017/04/25 23:10:36, megjablon wrote:
> Why do we care about cellular only here?

Yeah, not sure, reverting.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:158:
!params::IsLoFiCellularOnlyViaFlags())
On 2017/04/25 23:10:36, megjablon wrote:
> Use brackets

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(left):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:873:
DCHECK((params::IsIncludedInLoFiEnabledFieldTrial() ||
On 2017/04/25 23:10:36, megjablon wrote:
> Keep these DCHECKS? Is this cl just a intermediate state and alt-transform cl
> will be soon to follow and make it into M60?

This RecordAutoLoFiAccuracyRate() method is gone now. I have a very reduced
IsNetworkQualityProhibitivelySlow() to bridge to the next CL for alt-transforms
that does need to land in M60. 

Btw, I do wonder if I should put the lofi_effective_connection_type_threshold_
back in for this CL vs. wiring in the default threshold.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:637:
if (!params::IsLitePageFallbackEnabled())
On 2017/04/25 23:10:36, megjablon wrote:
> Once we have alt-transform should we even have IsLitePageFallbackEnabled()? My
> understanding is this new protocol was to support lite page fallback via the
> server, and my field trial/client impl was just so we could get the fallback
in
> sooner. If so, add a comment here to remove the check.

Adding TODO to revisit this topic. I had discussed with Ben a few weeks ago and
he wanted to keep client field trial control over fallback support.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:195:
// proxy server asking for them (via alt-transforms directive).
On 2017/04/25 23:10:36, megjablon wrote:
> I thought this didn't support alt-transforms yet and still checks ECT? 

True, hadn't backed this comment update out (it was anticipating next CL).

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(left):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:100:
// mode only on slow connections.
On 2017/04/25 23:10:36, megjablon wrote:
> Why this change?

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:28: // header if
needed. If a slow page preview is triggered, adds "lite-page" or
On 2017/04/25 23:10:37, megjablon wrote:
> s/If a slow page preview is/If a preview

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:54: // the page not
being heavy).
On 2017/04/25 23:10:36, megjablon wrote:
> (in spite of a good network connection, the page being blacklisted, or the
page
> not being heavy)?

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:59: // true if Lo-Fi
is enabled for |request| or Chrome session is in Lo-Fi
On 2017/04/25 23:10:36, megjablon wrote:
> This should still be 'and'

Done.

https://codereview.chromium.org/2802843003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:61: virtual bool
ShouldRecordLoFiUMA(const net::URLRequest& request) const = 0;
On 2017/04/25 23:10:36, megjablon wrote:
> Rename this to ShouldRecordPreviewsUMA? Since it does return true for Lite
Page
> users. Lite Pages use to be a subset of Lo-Fi, but now that we're starting to
> look at them as mostly separate previews types we should change that language
> wherever possible.

Done.

[commit-bot: I haz the power, 2017-04-26 20:00:14]

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

[commit-bot: I haz the power, 2017-04-26 20:00:15]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2017-04-26 20:13:52]

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

[dougarnett, 2017-04-26 20:13:52]

Ryan, please give a look

[dougarnett, 2017-04-28 22:10:00]

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

[commit-bot: I haz the power, 2017-04-28 22:10:37]

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-04-28 22:15:45]

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

[commit-bot: I haz the power, 2017-04-28 22:15:46]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dougarnett, 2017-04-28 23:08:20]

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

[commit-bot: I haz the power, 2017-04-28 23:08:59]

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-04-29 00:35:33]

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

[commit-bot: I haz the power, 2017-04-29 00:35:34]

Dry run: This issue passed the CQ dry run.

[bengr, 2017-05-01 15:51:31]

Description was changed from

==========
Update CPAT protocol to send lite-page transform acceptance with ect
header without client limiting by effective conneciton. In particular,
this removes NQE client checks for LitePage previews and removes
";if_heavy" header qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect.
We first need the server support for alt-transforms directive
to stop checking ect on client for LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol to send lite-page transform acceptance with ect
header without client limiting by effective connection. In particular,
this removes NQE client checks for LitePage previews and removes
";if_heavy" header qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect.
We first need the server support for alt-transforms directive
to stop checking ect on client for LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[bengr, 2017-05-01 15:52:30]

So what's the state of LoFi after this? Is it still using the client-side
ECT-based trigger?

[dougarnett, 2017-05-01 16:14:30]

On 2017/05/01 15:52:30, bengr wrote:
> So what's the state of LoFi after this? Is it still using the client-side
> ECT-based trigger?

Yes, the PreviewsState SERVER_LOFI_ON bit gets set based on ECT in addition
to flags/finch checks in DataReductionProxyConfig::ShouldEnableLoFi()

[dougarnett, 2017-05-01 16:34:37]

Description was changed from

==========
Update CPAT protocol to send lite-page transform acceptance with ect
header without client limiting by effective connection. In particular,
this removes NQE client checks for LitePage previews and removes
";if_heavy" header qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Does not yet add support for empty-image accept with ect.
We first need the server support for alt-transforms directive
to stop checking ect on client for LoFi. 

 
BUG=701802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update CPAT protocol to send lite-page transform acceptance with ect
header without client limiting by effective connection. In particular,
this removes NQE client checks for LitePage previews and removes
";if_heavy" header qualification.

Also replaces exp=ignore_preview_blacklist directive with new
exp=force_lite_page one.

Also adds some ECT header integration test coverage in lite_page.py.

Does not yet add support for empty-image accept with ect.
We first need the server support for alt-transforms directive
to stop checking ect on client for LoFi. 

 
BUG=701802,691135
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[bengr, 2017-05-01 16:53:15]

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:83: bool
resource_type_supports_empty_image =
Hmm. I don't see this being used anymore, but it should be, right?

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:154: //
Ensure lite page is requested in headers and enabled via flag.
This is very confusing. If lite pages are requested in command line flags and
the request already has c-p-a-t: lite-page, it seems we add lite-page to the
list of directives in c-p-a-t? Am I reading this wrong?

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(left):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:402:
TEST_F(ContentLoFiDeciderTest, LoFiControlFieldTrial) {
Please explain why each of these tests can be deleted. I.e., separate and spell
out where logic has been removed vs moved.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:310:
bool is_lofi_resource_type =
Are there tests that cover !is_lofi_resource_type?

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(left):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:533:
bool DataReductionProxyConfig::IsNetworkQualityProhibitivelySlow(
I must be missing something, but I thought you said offline that LoFi would
still be triggered by client logic after this CL. How is that possible if you
are removing all of the triggering logic?

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(left):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:251:
enum NetworkQualityAtLastQuery {
Give tbansal@ a heads up that all of this stuff is going away.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:194:
// Returns true when Lo-Fi Previews are allowed on the client.
previews? case?

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:813:
TEST_F(DataReductionProxyConfigTest, ShouldEnableLoFi) {
Would you mind breaking each one of these cases out into a separate test? We
should avoid the pattern of building up an array of test cases.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:825:
nullptr, std::string(), true, true, true, false,
#include <string>

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:828:
// Lo-Fi is disabled through command line switch is off and the user
I can't parse. Do you mean "switch being off?"

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:875:
// LitePage fallback support enables Lo-Fi. LoFi should be used
Is it Lo-Fi or LoFi?

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:930:
TEST_F(DataReductionProxyConfigTest, ShouldEnableLitePages) {
Likewise, please break out into separate tests.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:989:
base::CommandLine::ForCurrentProcess()->InitFromArgv(0, NULL);
You can put all of this stuff into a test helper.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1176:
bool lofi_field_trial;
Does this mean "in the enabled group of the lofi field trial?"

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:49:
const char kChromeProxyForceLitePageDirective[] = "exp=force_lite_page";
Add a TODO to factor out the data reduction proxy's experimental framework.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:102:
// preview experiment to force a lite page (in spite of current effective
Use the word "unconditionally" instead of naming conditions.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:33: const
net::URLRequest& request,
This doesn't need the whole request. It uses the resource type and the url
scheme, so it would be better to pass those in directly.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:53: // command line
flags being set (in spite of a good network connection, the
I would stay away from commenting on the types of signals we use to trigger lite
page. Maybe:

// Adds a directive to tell the server to return a Lite Page preview
unconditionally.

https://codereview.chromium.org/2802843003/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/common/lofi_decider.h:58: // Returns true
if the Previews specific UMA should be recorded. It is set to
Previews specific -> Previews-specific

Also, preview and previews should be case-consistent everywhere. Maybe use lower
case?

https://codereview.chromium.org/2802843003/diff/200001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2802843003/diff/200001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:366: previews_state = PREVIEWS_OFF;
What is the significance of this change?

https://codereview.chromium.org/2802843003/diff/200001/content/public/common/...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2802843003/diff/200001/content/public/common/...
content/public/common/previews_state.h:38: // TODO(dougarnett): Replace
PreviewsNoTransform with PreviewsOff in blink.
Reference a crbug.

https://codereview.chromium.org/2802843003/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2802843003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3745: // clear LoFi bit if not directed by
server to use LoFi.
What is "LoFi bit"? 

Also, rename cpct as chrome _proxy_content_transform_value.

https://codereview.chromium.org/2802843003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3750: if (cpct ==
kChromeProxyLitePageDirective) {
Why do we have Chrome-specific code in content? Add TODO and file a bug.

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

https://codereview.chromium.org/2802843003/diff/200001/tools/chrome_proxy/web...
tools/chrome_proxy/webdriver/lite_page.py:14: # provided when always-on.
is provided?

https://codereview.chromium.org/2802843003/diff/200001/tools/chrome_proxy/web...
tools/chrome_proxy/webdriver/lite_page.py:125: # Checks that the server does not
provide Lite Page for fast connection.
for -> for a

[dougarnett, 2017-05-01 21:41:45]

The proposed CPAT protocol is not approved yet and target post M-60. Will shelve
this CL for now.

[dougarnett, 2017-05-01 21:42:03]

dougarnett@chromium.org changed reviewers:
- bengr@chromium.org, megjablon@chromium.org, ryansturm@chromium.org