Change Lo-Fi bool to bitmask to support multiple Previews types

Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

TBR=mek@chromium.org
BUG=657997
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2484633004
Cr-Commit-Position: refs/heads/master@{#443303}
Committed: https://chromium.googlesource.com/chromium/src/+/caf312f64306f7490999d4587d7b8426b0a6aa67

[megjablon, 2016-11-08 00:45:16]

Description was changed from

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

to

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

[megjablon, 2016-11-11 00:34:03]

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

[commit-bot: I haz the power, 2016-11-11 00:34: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, 2016-11-11 00:45:08]

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

[commit-bot: I haz the power, 2016-11-11 00:45:09]

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

[megjablon, 2016-11-11 01:28:12]

Patchset #2 (id:20001) has been deleted

[megjablon, 2016-11-11 01:28:47]

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

[commit-bot: I haz the power, 2016-11-11 01:29:11]

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-11-11 01:38:30]

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

[commit-bot: I haz the power, 2016-11-11 01:38:30]

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

[megjablon, 2016-11-23 20:34:24]

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

[commit-bot: I haz the power, 2016-11-23 20:35:07]

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-11-23 21:01:57]

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

[commit-bot: I haz the power, 2016-11-23 21:01:58]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[megjablon, 2016-11-28 20:29:46]

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

[commit-bot: I haz the power, 2016-11-28 20:30:33]

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

[megjablon, 2016-11-28 20:38:56]

Patchset #3 (id:60001) has been deleted

[megjablon, 2016-11-28 20:39:00]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-28 20:59:34]

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

[commit-bot: I haz the power, 2016-11-28 20:59:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[megjablon, 2016-11-28 21:20:33]

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

[commit-bot: I haz the power, 2016-11-28 21:21:55]

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-11-28 21:44:29]

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

[commit-bot: I haz the power, 2016-11-28 21:44:30]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[megjablon, 2016-11-28 21:51:43]

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

[commit-bot: I haz the power, 2016-11-28 21:52:40]

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

[megjablon, 2016-11-28 21:55:49]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-28 22:20:47]

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

[commit-bot: I haz the power, 2016-11-28 22:20:49]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2016-11-28 22:32:07]

Patchset #2 (id:100001) has been deleted

[megjablon, 2016-11-28 22:32:11]

Patchset #1 (id:80001) has been deleted

[megjablon, 2016-11-28 22:45:36]

Description was changed from

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

to

==========
Change Lo-Fi bool to enum to support Client-side Lo-Fi

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

[megjablon, 2016-11-28 22:45:48]

Description was changed from

==========
Change Lo-Fi bool to enum to support Client-side Lo-Fi

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

to

==========
Change Lo-Fi bool to bitmask to support Client-side Lo-Fi

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

[megjablon, 2016-11-28 22:46:57]

Description was changed from

==========
Change Lo-Fi bool to bitmask to support Client-side Lo-Fi

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

to

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

This will support Client-side Lo-Fi and other Previews optimizations which can
be enabled in parallel. 

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

[megjablon, 2016-11-28 22:49:43]

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

[megjablon, 2016-11-28 22:49:52]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-11-28 22:50:48]

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-11-28 23:16:15]

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

[commit-bot: I haz the power, 2016-11-28 23:16:16]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[megjablon, 2016-11-28 23:52:51]

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

[commit-bot: I haz the power, 2016-11-28 23:54:16]

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

[megjablon, 2016-11-29 00:30:24]

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

[megjablon, 2016-11-29 00:30:29]

Patchset #2 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-11-29 00:31:57]

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-11-29 01:46:32]

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

[commit-bot: I haz the power, 2016-11-29 01:46:33]

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...)

[megjablon, 2016-11-29 22:27:54]

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

[commit-bot: I haz the power, 2016-11-29 22:28:42]

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-11-29 22:33:59]

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

[commit-bot: I haz the power, 2016-11-29 22:34:00]

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...)
  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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2016-11-29 22:35:12]

Patchset #2 (id:180001) has been deleted

[megjablon, 2016-11-29 22:35:17]

Patchset #1 (id:140001) has been deleted

[megjablon, 2016-11-29 22:45:41]

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

[commit-bot: I haz the power, 2016-11-29 22:46:36]

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

[megjablon, 2016-11-29 23:02:59]

Patchset #1 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-11-30 00:06:15]

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

[commit-bot: I haz the power, 2016-11-30 00:06:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2016-11-30 00:34:21]

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

[commit-bot: I haz the power, 2016-11-30 00:35:41]

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-11-30 02:08:21]

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

[commit-bot: I haz the power, 2016-11-30 02:08:22]

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

[megjablon, 2016-11-30 19:26:50]

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

[megjablon, 2016-11-30 19:26:54]

Patchset #1 (id:220001) has been deleted

[megjablon, 2016-11-30 19:27:14]

Patchset #1 (id:240001) has been deleted

[commit-bot: I haz the power, 2016-11-30 19:27:34]

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-11-30 19:32:50]

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

[commit-bot: I haz the power, 2016-11-30 19:32:51]

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...)

[megjablon, 2016-11-30 20:05:57]

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

[commit-bot: I haz the power, 2016-11-30 20:06:31]

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-11-30 23:36:50]

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

[commit-bot: I haz the power, 2016-11-30 23:36:51]

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_...)

[megjablon, 2016-12-01 20:21:07]

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

[commit-bot: I haz the power, 2016-12-01 20:21:44]

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

[megjablon, 2016-12-01 20:21:51]

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

[megjablon, 2016-12-01 20:21:52]

PTAL!

[megjablon, 2016-12-01 22:07:58]

Patchset #1 (id:260001) has been deleted

[megjablon, 2016-12-01 22:08:04]

Patchset #1 (id:280001) has been deleted

[RyanSturm, 2016-12-03 00:28:11]

https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:835: if
(previews_state & content::PREVIEWS_UNSPECIFIED) {
Can you add a comment that previews_state is content::PREVIEWS_OFF when
reloading with LoFi disabled? When a new preview is added, I want it clear that
either they have to follow the reload with lofi off pattern for the preview, or
they should be aware that they might have define a preview state for LOFI_OFF
that their preview type could ignore here.

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

https://codereview.chromium.org/2484633004/diff/320001/content/public/common/...
content/public/common/previews_state.h:15: PREVIEWS_UNSPECIFIED = 1 << 0, // Let
the browser process decide whether or
I don't think UNSPECIFIED is necessary, but I might not have thought it through.
Is it possible to have UNSPECIFIED set and have another bit set, or can 0
represent an unspecified state? It might be a good idea to have
PREVIEWS_UNSPECIFIED = 0 and do an explicit previews_state ==
PREVIEWS_UNSPECIFIED as opposed to a bitwise and for that case.

[megjablon, 2016-12-03 00:48:50]

Description was changed from

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

This will support Client-side Lo-Fi and other Previews optimizations which can
be enabled in parallel. 

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

to

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

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

[megjablon, 2016-12-05 21:09:58]

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

[commit-bot: I haz the power, 2016-12-05 21:10:40]

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-12-05 21:26:43]

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

[commit-bot: I haz the power, 2016-12-05 21:26:45]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2016-12-05 21:42:08]

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

[megjablon, 2016-12-05 21:42:14]

Patchset #3 (id:340001) has been deleted

[commit-bot: I haz the power, 2016-12-05 21:43:09]

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

[megjablon, 2016-12-06 00:59:17]

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

[megjablon, 2016-12-06 00:59:18]

+nasko: content/*

https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:835: if
(previews_state & content::PREVIEWS_UNSPECIFIED) {
On 2016/12/03 00:28:11, Ryan Sturm wrote:
> Can you add a comment that previews_state is content::PREVIEWS_OFF when
> reloading with LoFi disabled? When a new preview is added, I want it clear
that
> either they have to follow the reload with lofi off pattern for the preview,
or
> they should be aware that they might have define a preview state for LOFI_OFF
> that their preview type could ignore here.

Added the comment to resource_dispatcher_host_impl.cc since removing
PREVIEWS_UNSPECIFIED allowed me to go back to the original logic where
ChromeResourceDispatcherHostDelegate::GetPreviewsState isn't called unless the
state is "unspecified" (now 0).

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

https://codereview.chromium.org/2484633004/diff/320001/content/public/common/...
content/public/common/previews_state.h:15: PREVIEWS_UNSPECIFIED = 1 << 0, // Let
the browser process decide whether or
On 2016/12/03 00:28:11, Ryan Sturm wrote:
> I don't think UNSPECIFIED is necessary, but I might not have thought it
through.
> Is it possible to have UNSPECIFIED set and have another bit set, or can 0
> represent an unspecified state? It might be a good idea to have
> PREVIEWS_UNSPECIFIED = 0 and do an explicit previews_state ==
> PREVIEWS_UNSPECIFIED as opposed to a bitwise and for that case.

Done.

[commit-bot: I haz the power, 2016-12-06 01:29:52]

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

[commit-bot: I haz the power, 2016-12-06 01:29:54]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-12-06 19:39:30]

lgtm % 1 maybe nit

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

https://codereview.chromium.org/2484633004/diff/360001/content/public/common/...
content/public/common/previews_state.h:16: SERVER_LOFI_ON = 1 << 0,       //
Request a Lo-Fi version of the resource
See comment on blink version of this.

https://codereview.chromium.org/2484633004/diff/360001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2484633004/diff/360001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLRequest.h:142: enum PreviewsState {
maybe nit: I can't find anything in the style guide, but it might be more clear
to replace various 0's throughout the CL with:
PreviewsUnspecified and defined PreviewsUnspecified as 0 here. Same for content
side of things if you decide to change it.

[megjablon, 2016-12-06 21:10:53]

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

[commit-bot: I haz the power, 2016-12-06 21:11:31]

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

[megjablon, 2016-12-06 21:13:37]

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

[megjablon, 2016-12-06 21:23:46]

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

[megjablon, 2016-12-06 21:23:47]

+pfeldman: content/*

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

https://codereview.chromium.org/2484633004/diff/360001/content/public/common/...
content/public/common/previews_state.h:16: SERVER_LOFI_ON = 1 << 0,       //
Request a Lo-Fi version of the resource
On 2016/12/06 19:39:30, Ryan Sturm wrote:
> See comment on blink version of this.

Done.

https://codereview.chromium.org/2484633004/diff/360001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2484633004/diff/360001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLRequest.h:142: enum PreviewsState {
On 2016/12/06 19:39:30, Ryan Sturm wrote:
> maybe nit: I can't find anything in the style guide, but it might be more
clear
> to replace various 0's throughout the CL with:
> PreviewsUnspecified and defined PreviewsUnspecified as 0 here. Same for
content
> side of things if you decide to change it.

I see this done elsewhere throughout the code, so I don't think there's a style
issue with it. Done.

[commit-bot: I haz the power, 2016-12-06 23:20:25]

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

[commit-bot: I haz the power, 2016-12-06 23:20:26]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-12-08 22:19:30]

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

[nasko, 2016-12-08 22:19:34]

Overall I find the "Previews" naming to be confusing and find that using
LoFiState to be perfectly fine, even when adding the additional state.

Also, please use the proper types when passing the argument around.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:674: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:183: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:148: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:79: int previews_state,
PreviewsState

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

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:343: int previews_state =
PREVIEWS_UNSPECIFIED;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1130: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:131: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:574: int
last_navigation_previews_state() const {
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:1050: int
last_navigation_previews_state_;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:144: int
GetPreviewsState(
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:148: return 0;
Use the symbolic name - PREVIEWS_UNSPECIFIED or PREVIEWS_OFF, depending on which
one is the appropriate.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_browsertest.cc:645: int
GetPreviewsState(
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_browsertest.cc:681: int
previews_state_;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:360: int
GetPreviewsState(int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1845: int
previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2409: int
previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:322: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:639: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:73: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:101: int GetPreviewsState()
const override;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:236: int previews_state_;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
File content/child/weburlresponse_extradata_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
content/child/weburlresponse_extradata_impl.h:71: int previews_state() const {
return previews_state_; }
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
content/child/weburlresponse_extradata_impl.h:72: void set_previews_state(int
previews_state) {
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
content/child/weburlresponse_extradata_impl.h:91: int previews_state_;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
content/common/navigation_params.cc:54: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
content/common/navigation_params.h:56: int previews_state,
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
content/common/navigation_params.h:109: int previews_state;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:124: // Asks the
embedder for the Previews State which says which previews should
nit: PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:126: // unspecified
Previews state of zero.
The state is not zero, it has a meaningful name. Zero is the underlying
representation used for it.

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:127: virtual int
GetPreviewsState(const net::URLRequest& url_request,
Why isn't the return value PreviewsState?

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_request_info.h:52: int previews_state);
PreviewsState?

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

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/previews_state.h:13: // the resource.
Please explain a bit more what Previews is. As it is, the comment doesn't give
me a good idea of what it is.
Alternatively, point to a publicly accessible design document explaining the
concept.

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/previews_state.h:14: enum PreviewsState {
Honestly, Previews does not convey to me the meaning of what LoFi does.

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/previews_state.h:17: SERVER_LOFI_ON = 1 << 0,   // Request
a Lo-Fi version of the resource
Shouldn't the members be named using Previews instead of LoFi? Having both names
commingled leads to confusion.

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/resource_response_info.h:145: int previews_state;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/rendere...
content/public/renderer/render_frame.h:233: virtual int GetPreviewsState() const
= 0;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/test/re...
content/public/test/render_view_test.cc:519:
FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), 0,
PREVIEWS_UNSPECIFIED

https://codereview.chromium.org/2484633004/diff/380001/content/public/test/re...
content/public/test/render_view_test.cc:655: GURL(), 0, base::TimeTicks::Now(),
"GET", nullptr);
PREVIEWS_UNSPECIFIED

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.h:439: int GetPreviewsState() const override;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.h:1284: int previews_state_;
PreviewsState

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:87: void
SetPreviewsState(RenderFrameImpl* frame, int previews_state) {
PreviewsState

[megjablon, 2016-12-09 00:52:00]

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

[commit-bot: I haz the power, 2016-12-09 00:52:46]

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-12-09 01:12:31]

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

[commit-bot: I haz the power, 2016-12-09 01:12:33]

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_...)

[megjablon, 2016-12-09 18:57:22]

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

[commit-bot: I haz the power, 2016-12-09 18:57:56]

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

[megjablon, 2016-12-09 20:35:26]

Patchset #6 (id:420001) has been deleted

[megjablon, 2016-12-09 20:35:55]

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:674: int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:183: int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:148: int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:79: int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

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

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:343: int previews_state =
PREVIEWS_UNSPECIFIED;
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1130: int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:131: int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:574: int
last_navigation_previews_state() const {
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:1050: int
last_navigation_previews_state_;
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:144: int
GetPreviewsState(
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:148: return 0;
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> Use the symbolic name - PREVIEWS_UNSPECIFIED or PREVIEWS_OFF, depending on
which
> one is the appropriate.

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_browsertest.cc:645: int
GetPreviewsState(
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_browsertest.cc:681: int
previews_state_;
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:360: int
GetPreviewsState(int previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1845: int
previews_state,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2409: int
previews_state,
On 2016/12/08 22:19:32, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:322: int previews_state,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:639: int previews_state,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:73: int previews_state,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:101: int GetPreviewsState()
const override;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:236: int previews_state_;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
File content/child/weburlresponse_extradata_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
content/child/weburlresponse_extradata_impl.h:71: int previews_state() const {
return previews_state_; }
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
content/child/weburlresponse_extradata_impl.h:72: void set_previews_state(int
previews_state) {
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/child/weburlre...
content/child/weburlresponse_extradata_impl.h:91: int previews_state_;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
content/common/navigation_params.cc:54: int previews_state,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
content/common/navigation_params.h:56: int previews_state,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/common/navigat...
content/common/navigation_params.h:109: int previews_state;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:124: // Asks the
embedder for the Previews State which says which previews should
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> nit: PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:126: // unspecified
Previews state of zero.
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> The state is not zero, it has a meaningful name. Zero is the underlying
> representation used for it.

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:127: virtual int
GetPreviewsState(const net::URLRequest& url_request,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> Why isn't the return value PreviewsState?

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/browser...
content/public/browser/resource_request_info.h:52: int previews_state);
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState?

Done.

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

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/previews_state.h:13: // the resource.
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> Please explain a bit more what Previews is. As it is, the comment doesn't give
> me a good idea of what it is.
> Alternatively, point to a publicly accessible design document explaining the
> concept.

I added the description we use elsewhere. There isn't a public design doc, and
since I'm not the owner I can't change the access.

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/previews_state.h:14: enum PreviewsState {
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> Honestly, Previews does not convey to me the meaning of what LoFi does.

Lo-Fi is a type of Preview. Lo-Fi used to be the only optimization of it's kind,
but we're working on other page optimizations that all fit into this same
category. Previews are optimizations that change the format and content of web
pages to improve data savings and / or performance. They can be applied
individually or in combination. This enum will soon include Lite Pages, which
are currently just using the Lo-Fi bool, but should be considered their own
optimization. Other preview types will also be showing an offline page on slow
connections and possibly javascript disable.

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/previews_state.h:17: SERVER_LOFI_ON = 1 << 0,   // Request
a Lo-Fi version of the resource
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> Shouldn't the members be named using Previews instead of LoFi? Having both
names
> commingled leads to confusion.

These are the previews types that can be applied individually or in combination.

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/common/...
content/public/common/resource_response_info.h:145: int previews_state;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/rendere...
content/public/renderer/render_frame.h:233: virtual int GetPreviewsState() const
= 0;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/public/test/re...
content/public/test/render_view_test.cc:519:
FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), 0,
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PREVIEWS_UNSPECIFIED

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/public/test/re...
content/public/test/render_view_test.cc:655: GURL(), 0, base::TimeTicks::Now(),
"GET", nullptr);
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PREVIEWS_UNSPECIFIED

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.h:439: int GetPreviewsState() const override;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.h:1284: int previews_state_;
On 2016/12/08 22:19:33, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/2484633004/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:87: void
SetPreviewsState(RenderFrameImpl* frame, int previews_state) {
On 2016/12/08 22:19:34, nasko (very slow til Dec 13th) wrote:
> PreviewsState

Done.

[commit-bot: I haz the power, 2016-12-09 21:11:17]

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

[commit-bot: I haz the power, 2016-12-09 21:11:19]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-12-12 23:05:41]

Few more nits, but looks mostly there.

https://codereview.chromium.org/2484633004/diff/400001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2484633004/diff/400001/content/common/navigat...
content/common/navigation_params.cc:12: #include
"content/public/common/previews_state.h"
Why was this include removed? It is needed for PreviewsState, isn't it? We
should not depend on transitive includes.

https://codereview.chromium.org/2484633004/diff/400001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2484633004/diff/400001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.cc:10: #include
"content/public/common/previews_state.h"
This include should also stay.

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

https://codereview.chromium.org/2484633004/diff/440001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:337: // Determine if previews
should be used for the navigation.
nit: Previews

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
File content/browser/loader/async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
content/browser/loader/async_resource_handler_unittest.cc:191: PREVIEWS_OFF,    
             // previews_state
nit: Needs to be aligned with the rest.

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (left):

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_browsertest.cc:788:
ShouldEnableLoFiModeReloadDisableLoFi) {
What is the new behavior for when reloading? Can we have a test to ensure it
doesn't break?

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

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
content/public/common/previews_state.h:15: // determines which Preview types to
request.
nit: Previews?

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
content/public/common/previews_state.h:25: MAX_PREVIEWS_STATE = 1 << 3
You could use LAST pattern, which will be equal to the PREVIEWS_OFF values,
since we don't have to allow 1 << 3 as a value valid.

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
content/public/common/resource_response_info.h:145: // Which Previews versions
of the resource the request was for.
nit: version?

https://codereview.chromium.org/2484633004/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2484633004/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLRequest.h:148: PreviewsOff = 1 << 2    
 // Request a normal (non-Preview) version of
Can we add static checks to ensure the two enums don't get out of sync? An
example you can refer to is
https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?r....

[megjablon, 2016-12-13 01:13:32]

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

[commit-bot: I haz the power, 2016-12-13 01:14:58]

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

[megjablon, 2016-12-13 03:03:13]

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

[megjablon, 2016-12-13 03:03:14]

thestig: chrome/browser/*

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

https://codereview.chromium.org/2484633004/diff/440001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:337: // Determine if previews
should be used for the navigation.
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> nit: Previews

Done.

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
File content/browser/loader/async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
content/browser/loader/async_resource_handler_unittest.cc:191: PREVIEWS_OFF,    
             // previews_state
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> nit: Needs to be aligned with the rest.

Done.

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (left):

https://codereview.chromium.org/2484633004/diff/440001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_browsertest.cc:788:
ShouldEnableLoFiModeReloadDisableLoFi) {
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> What is the new behavior for when reloading? Can we have a test to ensure it
> doesn't break?

Added this back in. It was removed because my previous code moved checking the
lofi/previews state into Chrome, so on reload we would expect GetPreviewsState
to still be called.

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

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
content/public/common/previews_state.h:15: // determines which Preview types to
request.
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> nit: Previews?

Done.

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
content/public/common/previews_state.h:25: MAX_PREVIEWS_STATE = 1 << 3
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> You could use LAST pattern, which will be equal to the PREVIEWS_OFF values,
> since we don't have to allow 1 << 3 as a value valid.

Done.

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2484633004/diff/440001/content/public/common/...
content/public/common/resource_response_info.h:145: // Which Previews versions
of the resource the request was for.
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> nit: version?

Done.

https://codereview.chromium.org/2484633004/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2484633004/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLRequest.h:148: PreviewsOff = 1 << 2    
 // Request a normal (non-Preview) version of
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> Can we add static checks to ensure the two enums don't get out of sync? An
> example you can refer to is
>
https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?r....

Excellent idea. I added this to render_frame_impl.cc. Please let me know if it
should live elsewhere.

[megjablon, 2016-12-13 03:06:18]

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

[megjablon, 2016-12-13 03:06:19]

mek: extensions/*

[commit-bot: I haz the power, 2016-12-13 04:40:26]

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

[commit-bot: I haz the power, 2016-12-13 04:40:27]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-12-14 00:16:42]

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

https://codereview.chromium.org/2484633004/diff/460001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5518: // are kept in sync.
This doesn't seem to fit right here, as it splits Find functionality. I'd
suggest trying to put this in a file closer to where Previews is defined. I
don't know whether it would work putting it in the header file directly. If not,
move it closer to the code that sets/uses the enum.

[megjablon, 2016-12-14 01:08:45]

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

[commit-bot: I haz the power, 2016-12-14 01:09:26]

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

[megjablon, 2016-12-14 01:10:32]

https://codereview.chromium.org/2484633004/diff/400001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2484633004/diff/400001/content/common/navigat...
content/common/navigation_params.cc:12: #include
"content/public/common/previews_state.h"
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> Why was this include removed? It is needed for PreviewsState, isn't it? We
> should not depend on transitive includes.

Sorry didn't see these comments before. It was added to the .h

https://codereview.chromium.org/2484633004/diff/400001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2484633004/diff/400001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.cc:10: #include
"content/public/common/previews_state.h"
On 2016/12/12 23:05:41, nasko (very slow til Dec 13th) wrote:
> This include should also stay.

Same here. Added to the .h

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

https://codereview.chromium.org/2484633004/diff/460001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5518: // are kept in sync.
On 2016/12/14 00:16:42, nasko (very slow til Dec 13th) wrote:
> This doesn't seem to fit right here, as it splits Find functionality. I'd
> suggest trying to put this in a file closer to where Previews is defined. I
> don't know whether it would work putting it in the header file directly. If
not,
> move it closer to the code that sets/uses the enum.

moved to previews_state.h

[commit-bot: I haz the power, 2016-12-14 01:12:44]

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

[commit-bot: I haz the power, 2016-12-14 01:12:46]

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...)

[megjablon, 2016-12-14 19:14:31]

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

[commit-bot: I haz the power, 2016-12-14 19:15: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, 2016-12-14 20:29:31]

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

[commit-bot: I haz the power, 2016-12-14 20:29:34]

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

[megjablon, 2016-12-14 20:52:19]

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

[commit-bot: I haz the power, 2016-12-14 20:53:10]

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

[megjablon, 2016-12-14 21:13:46]

Patchset #9 (id:500001) has been deleted

[commit-bot: I haz the power, 2016-12-14 21:36:03]

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

[commit-bot: I haz the power, 2016-12-14 21:36:04]

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, 2016-12-14 22:46:00]

Patchset #9 (id:520001) has been deleted

[megjablon, 2016-12-14 22:47:54]

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

[commit-bot: I haz the power, 2016-12-14 22:48:48]

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

[nasko, 2016-12-15 01:09:11]

I should say it for the record - the name "Previews" is mighty confusing to me.
However, codewise LGTM.

[commit-bot: I haz the power, 2016-12-15 02:26:43]

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

[commit-bot: I haz the power, 2016-12-15 02:26:45]

Dry run: This issue passed the CQ dry run.

[megjablon, 2016-12-15 18:10:40]

megjablon@chromium.org changed reviewers:
- thestig@chromium.org

[megjablon, 2016-12-15 18:12:18]

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

[megjablon, 2016-12-15 18:12:19]

-thestig 
+thakis: chrome/browser/*

[megjablon, 2017-01-09 22:27:23]

On 2016/12/15 18:12:19, megjablon wrote:
> -thestig 
> +thakis: chrome/browser/*

A friendly ping :)

[Nico, 2017-01-09 22:35:03]

one repeated nit, and one real question:

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:444:
request.get(), resource_type, nullptr, 1, /* render_process_id */
nit: if, instead of putting the comment at the end, you say `...,
/*render_process_id=*/1, /*render_view_id=*/-1, ...` then clang-format will wrap
this a bit nicer

(also, thanks to this nice change, the PREVIEWS_OFF name is now self-explaining
and doesn't need a comment anymore)

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc:68:
content::PREVIEWS_OFF);  // previews_state
nit: remove now-useless comment for PREVIEWS_OFF

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:162:
content::PREVIEWS_OFF);  // previews_state
likewise; also like in the other places consider /*foo=*/ prefix comments
instead

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
chrome/browser/extensions/extension_protocols_unittest.cc:150:
content::PREVIEWS_OFF);  // previews_state
ditto

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h:86:
content::PreviewsState GetPreviewsState(
I'm confused about this. The enum definition looks like the states are mutually
exclusive and this returns the enum and not an unsigned, yet the CL description
says "bitmask" and the checks use & instead of ==. Is this a bitmask, or no?

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/chrome_data_use_group_provider_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/chrome_data_use_group_provider_unittest.cc:38:
content::PREVIEWS_OFF  /* previews_state */);
ditto

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/chrome_data_use_group_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/chrome_data_use_group_unittest.cc:33:
content::PREVIEWS_OFF /* previews_state */);
ditto

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle_unittest.cc:226:
content::PREVIEWS_OFF); // previews_state
ditto

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc:102:
content::PREVIEWS_OFF);  // previews_state
ditto

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/search/...
File chrome/browser/search/iframe_source_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/search/...
chrome/browser/search/iframe_source_unittest.cc:109: content::PREVIEWS_OFF);  //
previews_state
ditto

https://codereview.chromium.org/2484633004/diff/540001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc:80:
content::SERVER_LOFI_ON);  // previews_state
ditto

[megjablon, 2017-01-10 20:21:03]

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

[commit-bot: I haz the power, 2017-01-10 20:22:09]

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-10 20:43:27]

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

[commit-bot: I haz the power, 2017-01-10 20:43:29]

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

[megjablon, 2017-01-10 20:59:51]

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

[megjablon, 2017-01-10 21:00:19]

Patchset #11 (id:580001) has been deleted

[commit-bot: I haz the power, 2017-01-10 21:00: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-10 22:23:15]

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

[commit-bot: I haz the power, 2017-01-10 22:23:17]

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...)

[megjablon, 2017-01-10 22:29:43]

Patchset #11 (id:600001) has been deleted

[megjablon, 2017-01-10 22:30:01]

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:444:
request.get(), resource_type, nullptr, 1, /* render_process_id */
On 2017/01/09 22:35:03, Nico wrote:
> nit: if, instead of putting the comment at the end, you say `...,
> /*render_process_id=*/1, /*render_view_id=*/-1, ...` then clang-format will
wrap
> this a bit nicer
> 
> (also, thanks to this nice change, the PREVIEWS_OFF name is now
self-explaining
> and doesn't need a comment anymore)

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc:68:
content::PREVIEWS_OFF);  // previews_state
On 2017/01/09 22:35:03, Nico wrote:
> nit: remove now-useless comment for PREVIEWS_OFF

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:162:
content::PREVIEWS_OFF);  // previews_state
On 2017/01/09 22:35:03, Nico wrote:
> likewise; also like in the other places consider /*foo=*/ prefix comments
> instead

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/extensi...
chrome/browser/extensions/extension_protocols_unittest.cc:150:
content::PREVIEWS_OFF);  // previews_state
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h:86:
content::PreviewsState GetPreviewsState(
On 2017/01/09 22:35:03, Nico wrote:
> I'm confused about this. The enum definition looks like the states are
mutually
> exclusive and this returns the enum and not an unsigned, yet the CL
description
> says "bitmask" and the checks use & instead of ==. Is this a bitmask, or no?

The states aren't mutually exclusive. Server Lo-Fi and client Lo-Fi can both be
enabled at the same time. That's what this change is to help with. We need to
move Lo-Fi under Previews as a type, but also allow multiple previews types to
be set at once. This was initially an int, but nasko asked for me to use the
type, and I've seen it done both ways throughout the code.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/chrome_data_use_group_provider_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/chrome_data_use_group_provider_unittest.cc:38:
content::PREVIEWS_OFF  /* previews_state */);
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/chrome_data_use_group_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/chrome_data_use_group_unittest.cc:33:
content::PREVIEWS_OFF /* previews_state */);
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle_unittest.cc:226:
content::PREVIEWS_OFF); // previews_state
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc:102:
content::PREVIEWS_OFF);  // previews_state
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/search/...
File chrome/browser/search/iframe_source_unittest.cc (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/search/...
chrome/browser/search/iframe_source_unittest.cc:109: content::PREVIEWS_OFF);  //
previews_state
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2484633004/diff/540001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/540001/components/data_reduct...
components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc:80:
content::SERVER_LOFI_ON);  // previews_state
On 2017/01/09 22:35:03, Nico wrote:
> ditto

Done.

[Nico, 2017-01-11 01:14:47]

lgtm, thanks.

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h:86:
content::PreviewsState GetPreviewsState(
On 2017/01/10 22:30:01, megjablon wrote:
> On 2017/01/09 22:35:03, Nico wrote:
> > I'm confused about this. The enum definition looks like the states are
> mutually
> > exclusive and this returns the enum and not an unsigned, yet the CL
> description
> > says "bitmask" and the checks use & instead of ==. Is this a bitmask, or no?
> 
> The states aren't mutually exclusive. Server Lo-Fi and client Lo-Fi can both
be
> enabled at the same time. That's what this change is to help with. We need to
> move Lo-Fi under Previews as a type, but also allow multiple previews types to
> be set at once. This was initially an int, but nasko asked for me to use the
> type, and I've seen it done both ways throughout the code. 

I think some integer type is more natural for a bit mask, but ok. Add a comment
"Returns bitmask of potentially several PreviewsState comment" then maybe.

https://codereview.chromium.org/2484633004/diff/620001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/620001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc:66:
false,                   // allow_download
nit: /*foo=*/ comments here too

[megjablon, 2017-01-11 19:36:52]

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

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

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-11 19:38:57]

megjablon@chromium.org changed reviewers:
- pfeldman@chromium.org

[megjablon, 2017-01-11 19:39:28]

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

[megjablon, 2017-01-11 19:39:28]

Patchset #12 (id:640001) has been deleted

[commit-bot: I haz the power, 2017-01-11 19:40: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-01-11 19:42:39]

Description was changed from

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

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

to

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

TBR=mek@chromium.org

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

[megjablon, 2017-01-11 19:45:29]

Description was changed from

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

TBR=mek@chromium.org

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

to

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

TBR=mek@chromium.org
BUG=657997
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-11 20:00:56]

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

[commit-bot: I haz the power, 2017-01-11 20:00:58]

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

[megjablon, 2017-01-11 22:35:05]

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

[commit-bot: I haz the power, 2017-01-11 22:35:56]

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-11 22:56:11]

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

[commit-bot: I haz the power, 2017-01-11 22:56:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-01-11 23:15:23]

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

[commit-bot: I haz the power, 2017-01-11 23:16:10]

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-12 03:18:07]

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

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

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-01-12 17:56:29]

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h:86:
content::PreviewsState GetPreviewsState(
On 2017/01/11 01:14:47, Nico wrote:
> On 2017/01/10 22:30:01, megjablon wrote:
> > On 2017/01/09 22:35:03, Nico wrote:
> > > I'm confused about this. The enum definition looks like the states are
> > mutually
> > > exclusive and this returns the enum and not an unsigned, yet the CL
> > description
> > > says "bitmask" and the checks use & instead of ==. Is this a bitmask, or
no?
> > 
> > The states aren't mutually exclusive. Server Lo-Fi and client Lo-Fi can both
> be
> > enabled at the same time. That's what this change is to help with. We need
to
> > move Lo-Fi under Previews as a type, but also allow multiple previews types
to
> > be set at once. This was initially an int, but nasko asked for me to use the
> > type, and I've seen it done both ways throughout the code. 
> 
> I think some integer type is more natural for a bit mask, but ok. Add a
comment
> "Returns bitmask of potentially several PreviewsState comment" then maybe.

Done.

https://codereview.chromium.org/2484633004/diff/620001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc
(right):

https://codereview.chromium.org/2484633004/diff/620001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc:66:
false,                   // allow_download
On 2017/01/11 01:14:47, Nico wrote:
> nit: /*foo=*/ comments here too

Done.

[megjablon, 2017-01-12 18:39:38]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2017-01-12 18:39:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, nasko@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2484633004/#ps680001
(title: "fix ContentResourceProviderTest")

[commit-bot: I haz the power, 2017-01-12 18:40:19]

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-12 18:47:54]

CQ is committing da patch.

Bot data: {"patchset_id": 680001, "attempt_start_ts": 1484246378716820,
"parent_rev": "8c15a423974bf654c237bc27a2ac12cca22cccd2", "commit_rev":
"caf312f64306f7490999d4587d7b8426b0a6aa67"}

[commit-bot: I haz the power, 2017-01-12 18:48:38]

Description was changed from

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

TBR=mek@chromium.org
BUG=657997
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Change Lo-Fi bool to bitmask to support multiple Previews types.

In order to support Client-side Lo-Fi and other Previews optimizations
which can be enabled in parallel, change the Lo-Fi bool to a Previews
bitmask.

TBR=mek@chromium.org
BUG=657997
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2484633004
Cr-Commit-Position: refs/heads/master@{#443303}
Committed:
https://chromium.googlesource.com/chromium/src/+/caf312f64306f7490999d4587d7b...
==========

[commit-bot: I haz the power, 2017-01-12 18:48:41]

Committed patchset #13 (id:680001) as
https://chromium.googlesource.com/chromium/src/+/caf312f64306f7490999d4587d7b...