Create a Lite Page bit for previews and fallback to Lo-Fi

Create a Lite Page bit for previews and fallback to Lo-Fi

If a lite-page is requested and the server does not provide one, Chrome
should request a Lo-Fi image on subresources. However, Chrome should
not request empty-image on the subresource requests of a Weblite page.

BUG=660212
Review-Url: https://codereview.chromium.org/2642793005
Cr-Commit-Position: refs/heads/master@{#454638}
Committed: https://chromium.googlesource.com/chromium/src/+/3f4bae9092c2ac1443f2c26867b7fad7f10df1ca

[megjablon, 2017-01-19 06:41:33]

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

[commit-bot: I haz the power, 2017-01-19 06:41:49]

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

[megjablon, 2017-01-19 06:50:59]

Patchset #1 (id:1) has been deleted

[megjablon, 2017-01-19 06:51:17]

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

[commit-bot: I haz the power, 2017-01-19 06:51:39]

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-01-19 08:18:56]

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

[commit-bot: I haz the power, 2017-01-19 08:18:57]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-02-11 00:19:01]

Description was changed from

==========
Create a Lite Page bit for previews and fallback to Lo-Fi

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

to

==========
Create a Lite Page bit for previews and fallback to Lo-Fi

BUG=660212
==========

[megjablon, 2017-02-11 00:24:45]

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

[commit-bot: I haz the power, 2017-02-11 00:25:20]

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-02-11 00:28:21]

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

[commit-bot: I haz the power, 2017-02-11 00:28:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[megjablon, 2017-02-11 00:31:54]

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

[megjablon, 2017-02-11 00:32:05]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-11 00:33:05]

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

[megjablon, 2017-02-11 00:38:54]

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

[commit-bot: I haz the power, 2017-02-11 00:39:21]

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

[megjablon, 2017-02-11 00:39:48]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-11 06:40:23]

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

[commit-bot: I haz the power, 2017-02-11 06:40:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[megjablon, 2017-02-13 19:34:06]

Description was changed from

==========
Create a Lite Page bit for previews and fallback to Lo-Fi

BUG=660212
==========

to

==========
Create a Lite Page bit for previews and fallback to Lo-Fi

If a lite-page is requested and the server does not provide one, Chrome
should request a Lo-Fi image on subresources. However, Chrome should
not request empty-image on the subresource requests of a Weblite page.

BUG=660212
==========

[megjablon, 2017-02-13 19:42:30]

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

[megjablon, 2017-02-13 19:42:31]

PTAL

[bengr, 2017-02-15 00:03:47]

lgtm with minor suggestions.

https://codereview.chromium.org/2642793005/diff/60001/chrome/browser/loader/c...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2642793005/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:866:
content::PreviewsState previews_state = content::PREVIEWS_UNSPECIFIED;
Why do you need separate values for UNSPECIFIED and OFF?

https://codereview.chromium.org/2642793005/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:868: if
(data_reduction_proxy_io_data &&
Style nit:

if (drp_io_data) {
  if (drp_io_data->ShouldEnableFoo(request)
    previews_state |= LOFI_ON;
  if (drp_io_data->ShouldEnableBar(request)
    previews_state |= LITE_PAGE_ON;

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

https://codereview.chromium.org/2642793005/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:115: if
(!(request_info->GetPreviewsState() & content::SERVER_LITE_PAGE_ON))
Add a comment:
// Since a lite page was not triggered client side, ask the server to
// provide a lite page only if the page is otherwise data-heavy.

Add a similar comment before line 123.

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

https://codereview.chromium.org/2642793005/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:995:
return ShouldEnableLitePagesInternal(network_quality_estimator);
You could get rid of the temporary:
return ShouldEnableLitePagesInternal(
    request.context() ? request.context()->network_quality_estimator() :
        nullptr);

https://codereview.chromium.org/2642793005/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1044:
// TODO: Move the Lo-Fi flag to a flag supporting all Previews types.
TODO(megjablon):

[megjablon, 2017-02-16 04:29:08]

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

[megjablon, 2017-02-16 04:29:09]

nasko: content/* Thanks!

https://codereview.chromium.org/2642793005/diff/60001/chrome/browser/loader/c...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2642793005/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:866:
content::PreviewsState previews_state = content::PREVIEWS_UNSPECIFIED;
On 2017/02/15 00:03:47, bengr wrote:
> Why do you need separate values for UNSPECIFIED and OFF?

For requesting the original page with previews off. When we do that set
PREVIEWS_OFF on the navigation request so that we don't call the code to get the
preview state here.

https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...

https://codereview.chromium.org/2642793005/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:868: if
(data_reduction_proxy_io_data &&
On 2017/02/15 00:03:47, bengr wrote:
> Style nit:
> 
> if (drp_io_data) {
>   if (drp_io_data->ShouldEnableFoo(request)
>     previews_state |= LOFI_ON;
>   if (drp_io_data->ShouldEnableBar(request)
>     previews_state |= LITE_PAGE_ON;

Done.

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

https://codereview.chromium.org/2642793005/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:115: if
(!(request_info->GetPreviewsState() & content::SERVER_LITE_PAGE_ON))
On 2017/02/15 00:03:47, bengr wrote:
> Add a comment:
> // Since a lite page was not triggered client side, ask the server to
> // provide a lite page only if the page is otherwise data-heavy.
> 
> Add a similar comment before line 123.

Done.

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

https://codereview.chromium.org/2642793005/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:995:
return ShouldEnableLitePagesInternal(network_quality_estimator);
On 2017/02/15 00:03:47, bengr wrote:
> You could get rid of the temporary:
> return ShouldEnableLitePagesInternal(
>     request.context() ? request.context()->network_quality_estimator() :
>         nullptr);

Done.

https://codereview.chromium.org/2642793005/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1044:
// TODO: Move the Lo-Fi flag to a flag supporting all Previews types.
On 2017/02/15 00:03:47, bengr wrote:
> TODO(megjablon):

Removing since we should discuss how we want the handle flags for previews
offline. We should probably just have individual ones for each preview.

[nasko, 2017-02-16 18:10:02]

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
content/public/common/previews_state.h:17: typedef int PreviewsState;
Why does that need to stay around?

https://codereview.chromium.org/2642793005/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2642793005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3616:
WebString::fromUTF8("chrome-proxy-content-transform"))
Please define a string constant with a symbolic name and use that.

[megjablon, 2017-02-16 22:42:37]

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

[commit-bot: I haz the power, 2017-02-16 22:44: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-02-16 22:54:12]

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

[commit-bot: I haz the power, 2017-02-16 22:54:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[megjablon, 2017-02-16 23:03:03]

Patchset #3 (id:100001) has been deleted

[megjablon, 2017-02-16 23:03:30]

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

[commit-bot: I haz the power, 2017-02-16 23:05:27]

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-02-16 23:35:17]

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

[commit-bot: I haz the power, 2017-02-16 23:35:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[megjablon, 2017-02-16 23:55:05]

Patchset #3 (id:120001) has been deleted

[megjablon, 2017-02-17 00:17:20]

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

[commit-bot: I haz the power, 2017-02-17 00:18:39]

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-02-17 02:21:54]

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

[commit-bot: I haz the power, 2017-02-17 02:21:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[megjablon, 2017-02-17 20:17:53]

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
content/public/common/previews_state.h:17: typedef int PreviewsState;
On 2017/02/16 18:10:02, nasko wrote:
> Why does that need to stay around?

So that we can use these as a bitmask

https://codereview.chromium.org/2642793005/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2642793005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3616:
WebString::fromUTF8("chrome-proxy-content-transform"))
On 2017/02/16 18:10:02, nasko wrote:
> Please define a string constant with a symbolic name and use that.

Done.

[megjablon, 2017-02-22 22:17:42]

megjablon@chromium.org changed reviewers:
+ dstockwell@chromium.org, thestig@chromium.org

[megjablon, 2017-02-22 22:17:43]

thestig: chrome_resource_dispatcher_host_delegate.cc
dstockwell: WebURLRequest.h

[Lei Zhang, 2017-02-23 01:24:22]

lgtm

[nasko, 2017-02-28 00:30:46]

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
content/public/common/previews_state.h:17: typedef int PreviewsState;
On 2017/02/17 20:17:52, megjablon wrote:
> On 2017/02/16 18:10:02, nasko wrote:
> > Why does that need to stay around?
> 
> So that we can use these as a bitmask

It was a bitmask before while being an enum, wasn't it? I don't think we should
have two different types for the same thing.

[megjablon, 2017-02-28 00:44:23]

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
content/public/common/previews_state.h:17: typedef int PreviewsState;
On 2017/02/28 00:30:46, nasko (out until Feb 27th) wrote:
> On 2017/02/17 20:17:52, megjablon wrote:
> > On 2017/02/16 18:10:02, nasko wrote:
> > > Why does that need to stay around?
> > 
> > So that we can use these as a bitmask
> 
> It was a bitmask before while being an enum, wasn't it? I don't think we
should
> have two different types for the same thing.

Before we were only assigning it one value, but c++ won't allow setting both the
lite page and lofi bits using the PreviewsState type that you wanted in the
other reviews. I can switch using an int instead of PreviewsState everywhere,
but thought using a similar structure to accessibility mode would be cleaner.
https://cs.chromium.org/chromium/src/content/common/accessibility_mode_enums....

[nasko, 2017-03-01 18:30:42]

On 2017/02/28 00:44:23, megjablon wrote:
>
https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
> File content/public/common/previews_state.h (right):
> 
>
https://codereview.chromium.org/2642793005/diff/80001/content/public/common/p...
> content/public/common/previews_state.h:17: typedef int PreviewsState;
> On 2017/02/28 00:30:46, nasko (out until Feb 27th) wrote:
> > On 2017/02/17 20:17:52, megjablon wrote:
> > > On 2017/02/16 18:10:02, nasko wrote:
> > > > Why does that need to stay around?
> > > 
> > > So that we can use these as a bitmask
> > 
> > It was a bitmask before while being an enum, wasn't it? I don't think we
> should
> > have two different types for the same thing.
> 
> Before we were only assigning it one value, but c++ won't allow setting both
the
> lite page and lofi bits using the PreviewsState type that you wanted in the
> other reviews. I can switch using an int instead of PreviewsState everywhere,
> but thought using a similar structure to accessibility mode would be cleaner.
>
https://cs.chromium.org/chromium/src/content/common/accessibility_mode_enums....

Feels strange, but since it is an existing pattern, LGTM.

[dstockwell, 2017-03-02 03:35:02]

lgtm

[megjablon, 2017-03-03 00:10:01]

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

[commit-bot: I haz the power, 2017-03-03 00:10: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-03-03 01:16:56]

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

[commit-bot: I haz the power, 2017-03-03 01:16:58]

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

[megjablon, 2017-03-03 05:12:01]

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

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

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-03-03 06:16:52]

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

[commit-bot: I haz the power, 2017-03-03 06:16:53]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-03-03 18:55:07]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2017-03-03 18:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
thestig@chromium.org, dstockwell@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2642793005/#ps160001
(title: "remove unused frame_messages.h code")

[commit-bot: I haz the power, 2017-03-03 18:55:23]

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-03-03 19:01:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488567307010190,
"parent_rev": "f9aecd384eaaf9d50343b20c9a1bba61ed71f5dc", "commit_rev":
"3f4bae9092c2ac1443f2c26867b7fad7f10df1ca"}

[commit-bot: I haz the power, 2017-03-03 19:01:37]

Description was changed from

==========
Create a Lite Page bit for previews and fallback to Lo-Fi

If a lite-page is requested and the server does not provide one, Chrome
should request a Lo-Fi image on subresources. However, Chrome should
not request empty-image on the subresource requests of a Weblite page.

BUG=660212
==========

to

==========
Create a Lite Page bit for previews and fallback to Lo-Fi

If a lite-page is requested and the server does not provide one, Chrome
should request a Lo-Fi image on subresources. However, Chrome should
not request empty-image on the subresource requests of a Weblite page.

BUG=660212

Review-Url: https://codereview.chromium.org/2642793005
Cr-Commit-Position: refs/heads/master@{#454638}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f4bae9092c2ac1443f2c26867b7...
==========

[commit-bot: I haz the power, 2017-03-03 19:01:39]

Committed patchset #4 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3f4bae9092c2ac1443f2c26867b7...