Added support for heavy page previews transformations

Added support for requesting previews page transformations
on data heavy pages via the Chrome-Proxy-Accept-Transform
header.

BUG=657658
Committed: https://crrev.com/6d41561dab634214a3a875614bb703f6c0885e47
Cr-Commit-Position: refs/heads/master@{#426857}

[bengr, 2016-10-18 23:35:32]

Patchset #1 (id:1) has been deleted

[bengr, 2016-10-18 23:36:17]

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

[bengr, 2016-10-18 23:36:17]

ryansturm, PTAL.

[RyanSturm, 2016-10-19 18:58:57]

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

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:71:
(params::IsLoFiOnViaFlags() && params::AreLitePagesEnabledViaFlags()) ||
Is lofi being on via flags a requirement for lite pages being on via flags?

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:90: //
If in the preview field trial or the preview flag is enabled, only add the
s/preview/lite page/

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:93: //
If a preview was not triggered due to the network being slow, add the
nit: s/preview/server-side preview/  ?

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:93: //
If a preview was not triggered due to the network being slow, add the
This comment is somewhat confusing with the "not". Maybe just get rid of the
network details: "If a server-side preview was not triggered, add...

Move this part of the comment down to the code it applies to.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:107: if
(!request_info->IsUsingLoFi())
Just to verify, "if-heavy" is applied to lofi image requests here?

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

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:104:
// Requests a transformation only if the server determines that the page is
nit:s/page/resource/  ?

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:105:
// otherwise heavy (requires the network transfer of a lot of bytes). Added
nit: s/(requires the network transfer/ (i.e., the resource ordinarily requires
the network to transfer/

[bengr, 2016-10-19 23:24:02]

Patchset #2 (id:40001) has been deleted

[bengr, 2016-10-19 23:34:16]

Description was changed from

==========
Added support for requesting transform if-heavy

BUG=
==========

to

==========
Added support for requesting previews page transformations
on data heavy pages via the Chrome-Proxy-Accept-Transform
header.

BUG=657658
==========

[bengr, 2016-10-19 23:34:28]

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

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:71:
(params::IsLoFiOnViaFlags() && params::AreLitePagesEnabledViaFlags()) ||
On 2016/10/19 18:58:56, Ryan Sturm wrote:
> Is lofi being on via flags a requirement for lite pages being on via flags?

Yes, unfortunately.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:93: //
If a preview was not triggered due to the network being slow, add the
On 2016/10/19 18:58:56, Ryan Sturm wrote:
> This comment is somewhat confusing with the "not". Maybe just get rid of the
> network details: "If a server-side preview was not triggered, add...
> 
> Move this part of the comment down to the code it applies to.

Done.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:93: //
If a preview was not triggered due to the network being slow, add the
On 2016/10/19 18:58:56, Ryan Sturm wrote:
> nit: s/preview/server-side preview/  ?

Redundant imho. Headers only go to servers.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:107: if
(!request_info->IsUsingLoFi())
On 2016/10/19 18:58:56, Ryan Sturm wrote:
> Just to verify, "if-heavy" is applied to lofi image requests here?

Yes. To both LoFi and Lite Page previews, and only if the slow network detector
did NOT trigger.

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

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:104:
// Requests a transformation only if the server determines that the page is
On 2016/10/19 18:58:57, Ryan Sturm wrote:
> nit:s/page/resource/  ?

Nope, the server looks at the enclosing page. Easy for lite pages, harder for
LoFi.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:105:
// otherwise heavy (requires the network transfer of a lot of bytes). Added
On 2016/10/19 18:58:56, Ryan Sturm wrote:
> nit: s/(requires the network transfer/ (i.e., the resource ordinarily requires
> the network to transfer/

Done.

[RyanSturm, 2016-10-19 23:43:12]

lgtm.

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

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:71:
(params::IsLoFiOnViaFlags() && params::AreLitePagesEnabledViaFlags()) ||
On 2016/10/19 23:34:28, bengr wrote:
> On 2016/10/19 18:58:56, Ryan Sturm wrote:
> > Is lofi being on via flags a requirement for lite pages being on via flags?
> 
> Yes, unfortunately.

Acknowledged.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:90: //
If in the preview field trial or the preview flag is enabled, only add the
On 2016/10/19 18:58:56, Ryan Sturm wrote:
> s/preview/lite page/

Done.

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:107: if
(!request_info->IsUsingLoFi())
On 2016/10/19 23:34:28, bengr wrote:
> On 2016/10/19 18:58:56, Ryan Sturm wrote:
> > Just to verify, "if-heavy" is applied to lofi image requests here?
> 
> Yes. To both LoFi and Lite Page previews, and only if the slow network
detector
> did NOT trigger.

Acknowledged.

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

https://codereview.chromium.org/2424263002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:104:
// Requests a transformation only if the server determines that the page is
On 2016/10/19 23:34:28, bengr wrote:
> On 2016/10/19 18:58:57, Ryan Sturm wrote:
> > nit:s/page/resource/  ?
> 
> Nope, the server looks at the enclosing page. Easy for lite pages, harder for
> LoFi.

Weird. sgtm.

https://codereview.chromium.org/2424263002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2424263002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:244:
*request, data_reduction_proxy_config_->lofi_off(), headers);
Thanks; this makes a lot more sense now.

[bengr, 2016-10-20 16:10:37]

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

[bengr, 2016-10-20 16:10:43]

https://codereview.chromium.org/2424263002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2424263002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:244:
*request, data_reduction_proxy_config_->lofi_off(), headers);
On 2016/10/19 23:43:12, Ryan Sturm wrote:
> Thanks; this makes a lot more sense now.

Acknowledged.

[commit-bot: I haz the power, 2016-10-20 16:11:03]

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, 2016-10-20 17:57:36]

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

[commit-bot: I haz the power, 2016-10-20 17:57:37]

Dry run: This issue passed the CQ dry run.

[bengr, 2016-10-21 17:23:49]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2016-10-21 17:24:37]

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, 2016-10-21 19:06:36]

Description was changed from

==========
Added support for requesting previews page transformations
on data heavy pages via the Chrome-Proxy-Accept-Transform
header.

BUG=657658
==========

to

==========
Added support for requesting previews page transformations
on data heavy pages via the Chrome-Proxy-Accept-Transform
header.

BUG=657658
==========

[commit-bot: I haz the power, 2016-10-21 19:06:37]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-10-21 19:44:42]

Description was changed from

==========
Added support for requesting previews page transformations
on data heavy pages via the Chrome-Proxy-Accept-Transform
header.

BUG=657658
==========

to

==========
Added support for requesting previews page transformations
on data heavy pages via the Chrome-Proxy-Accept-Transform
header.

BUG=657658

Committed: https://crrev.com/6d41561dab634214a3a875614bb703f6c0885e47
Cr-Commit-Position: refs/heads/master@{#426857}
==========

[commit-bot: I haz the power, 2016-10-21 19:44:43]

Patchset 2 (id:??) landed as
https://crrev.com/6d41561dab634214a3a875614bb703f6c0885e47
Cr-Commit-Position: refs/heads/master@{#426857}