Moving previews code from components/ to chrome/

Moving previews code from components/ to chrome/

Allows support for looking in other components or chrome code to verify
if a preview is enabled and check the previews version. The layering
dynamic is that components such as d_r_p will depend on previews/ and
not the other way around, so these methods should be moved to chrome/
and relevant information should be injected.

BUG=700459
Review-Url: https://codereview.chromium.org/2739033005
Cr-Commit-Position: refs/heads/master@{#456511}
Committed: https://chromium.googlesource.com/chromium/src/+/e69df7ab2555ce8874972c419b13add8e0ec0c32

[RyanSturm, 2017-03-10 19:07:28]

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

[commit-bot: I haz the power, 2017-03-10 19:08: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-10 19:20:18]

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

[commit-bot: I haz the power, 2017-03-10 19:20:20]

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

[RyanSturm, 2017-03-10 19:26:35]

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

[commit-bot: I haz the power, 2017-03-10 19:27:17]

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

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

[commit-bot: I haz the power, 2017-03-10 20:20:44]

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

[RyanSturm, 2017-03-10 20:21:33]

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

[RyanSturm, 2017-03-10 20:21:34]

tbansal: PTAL, the context for this is getting the version and enabled state of
LoFi and WebLite well keeping the DEPS order d_r_p -> previews

[tbansal1, 2017-03-10 21:14:53]

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
File chrome/browser/previews/previews_service.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:24: // prohibitvely slow networks is
active.
This comment looks incorrect.

Or may be change the function name to be more descriptive.

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:37: // Returns the version of
preview treatment |type|. Should default to 0 if not
s/Should default/Defaults/

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:60: if (IsPreviewsTypeEnabled(type))
{
rm parens.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
File components/previews/core/previews_experiments.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_experiments.cc:5: #include
"components/previews/core/previews_experiments.h"
Would it make sense to rename this file as offline_previews since it deals with
a specific type of preview (i.e., offline)? IIUC, the params for all non-offline
previews is now in chrome/.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_experiments.cc:193: return
ClientSidePreviewsParamValue(kOfflinePagesSlowNetwork) ==
Can you add comments on what this is exactly checking for.

May be change the function name to
IsOfflinePreviewsEnabledForSlowNetworks()

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_experiments.cc:199: if
(!base::StringToInt(ClientSidePreviewsParamValue(kVersion), &version)) {
skip parens.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
File components/previews/core/previews_io_data.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_io_data.cc:100: // The blacklist will disallow
certain hosts for periods of time based on
add a newline before.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
File components/previews/core/previews_ui_service.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_ui_service.cc:17: PreviewsIsEnabledCallback
is_enabled_callback)
pass as const ref?

[RyanSturm, 2017-03-10 22:04:46]

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

[commit-bot: I haz the power, 2017-03-10 22:05:00]

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

[RyanSturm, 2017-03-10 22:06:30]

tbansal: PTAL, thanks

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
File chrome/browser/previews/previews_service.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:24: // prohibitvely slow networks is
active.
On 2017/03/10 21:14:53, tbansal1 wrote:
> This comment looks incorrect.
> 
> Or may be change the function name to be more descriptive.

Done.

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:37: // Returns the version of
preview treatment |type|. Should default to 0 if not
On 2017/03/10 21:14:53, tbansal1 wrote:
> s/Should default/Defaults/

Done.

https://codereview.chromium.org/2739033005/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:60: if (IsPreviewsTypeEnabled(type))
{
On 2017/03/10 21:14:53, tbansal1 wrote:
> rm parens.

Done.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
File components/previews/core/previews_experiments.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_experiments.cc:5: #include
"components/previews/core/previews_experiments.h"
On 2017/03/10 21:14:53, tbansal1 wrote:
> Would it make sense to rename this file as offline_previews since it deals
with
> a specific type of preview (i.e., offline)? IIUC, the params for all
non-offline
> previews is now in chrome/.

It's more like black list + offline, I think it's fine as is. If another small
preview wants to have it's own field trial in here, I wouldn't mind.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_experiments.cc:193: return
ClientSidePreviewsParamValue(kOfflinePagesSlowNetwork) ==
On 2017/03/10 21:14:53, tbansal1 wrote:
> Can you add comments on what this is exactly checking for.
> 

Done.

> May be change the function name to
> IsOfflinePreviewsEnabledForSlowNetworks()
> 

Ack.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_experiments.cc:199: if
(!base::StringToInt(ClientSidePreviewsParamValue(kVersion), &version)) {
On 2017/03/10 21:14:53, tbansal1 wrote:
> skip parens.

Done.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
File components/previews/core/previews_io_data.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_io_data.cc:100: // The blacklist will disallow
certain hosts for periods of time based on
On 2017/03/10 21:14:53, tbansal1 wrote:
> add a newline before.

Done.

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
File components/previews/core/previews_ui_service.cc (right):

https://codereview.chromium.org/2739033005/diff/20001/components/previews/cor...
components/previews/core/previews_ui_service.cc:17: PreviewsIsEnabledCallback
is_enabled_callback)
On 2017/03/10 21:14:53, tbansal1 wrote:
> pass as const ref?

Done.

[commit-bot: I haz the power, 2017-03-10 22:54:22]

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

[commit-bot: I haz the power, 2017-03-10 22:54:23]

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

[tbansal1, 2017-03-10 23:06:24]

lgtm % comments.

https://codereview.chromium.org/2739033005/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_service.cc (right):

https://codereview.chromium.org/2739033005/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:13: #include
"components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
rm this include?

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
File components/previews/core/previews_io_data_unittest.cc (right):

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
components/previews/core/previews_io_data_unittest.cc:160:
base::Bind(&CheckOfflineFieldTrial)));
why can we not pass previews::params::IsOfflinePreviewsEnabled() directly?

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
File components/previews/core/previews_opt_out_store_sql.cc (right):

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
components/previews/core/previews_opt_out_store_sql.cc:415:
enabled_previews_(std::move(enabled_previews)) {}
DCHECK(enabled_previews_);

[RyanSturm, 2017-03-13 19:53:40]

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

[commit-bot: I haz the power, 2017-03-13 19:54:30]

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-13 21:04:57]

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

[commit-bot: I haz the power, 2017-03-13 21:04:58]

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

[RyanSturm, 2017-03-13 21:12:41]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-03-13 21:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2739033005/#ps60001
(title: "tbansal comments")

[RyanSturm, 2017-03-13 21:12:47]

https://codereview.chromium.org/2739033005/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_service.cc (right):

https://codereview.chromium.org/2739033005/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:13: #include
"components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
On 2017/03/10 23:06:23, tbansal1 wrote:
> rm this include?

Done.

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
File components/previews/core/previews_io_data_unittest.cc (right):

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
components/previews/core/previews_io_data_unittest.cc:160:
base::Bind(&CheckOfflineFieldTrial)));
On 2017/03/10 23:06:23, tbansal1 wrote:
> why can we not pass previews::params::IsOfflinePreviewsEnabled() directly?

Type mismatch (takes a PreviewsType parameter)

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
File components/previews/core/previews_opt_out_store_sql.cc (right):

https://codereview.chromium.org/2739033005/diff/40001/components/previews/cor...
components/previews/core/previews_opt_out_store_sql.cc:415:
enabled_previews_(std::move(enabled_previews)) {}
On 2017/03/10 23:06:24, tbansal1 wrote:
> DCHECK(enabled_previews_);

Done.

[commit-bot: I haz the power, 2017-03-13 21:13:37]

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

[commit-bot: I haz the power, 2017-03-13 22:42:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489439561405500,
"parent_rev": "878b2e67ac952b00b2fcafc54867f61b25507923", "commit_rev":
"e69df7ab2555ce8874972c419b13add8e0ec0c32"}

[commit-bot: I haz the power, 2017-03-13 22:42:44]

Description was changed from

==========
Moving previews code from components/ to chrome/

Allows support for looking in other components or chrome code to verify
if a preview is enabled and check the previews version. The layering
dynamic is that components such as d_r_p will depend on previews/ and
not the other way around, so these methods should be moved to chrome/
and relevant information should be injected.

BUG=700459
==========

to

==========
Moving previews code from components/ to chrome/

Allows support for looking in other components or chrome code to verify
if a preview is enabled and check the previews version. The layering
dynamic is that components such as d_r_p will depend on previews/ and
not the other way around, so these methods should be moved to chrome/
and relevant information should be injected.

BUG=700459

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

[commit-bot: I haz the power, 2017-03-13 22:42:45]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e69df7ab2555ce8874972c419b13...

[yoichio, 2017-03-14 06:44:36]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2748033002/ by yoichio@chromium.org.

The reason for reverting is: Builders this step failed on:
Linux Tests [17 since first detection]
 1 test failed:
LoadAndLaunchExtensionBrowserTest.LoadAndLaunchExtension

Since:
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/....