predictors: Move the configuration of speculative prefetch to field trials.

predictors: Move the configuration of speculative prefetch to field trials.

BUG=631966
Review-Url: https://codereview.chromium.org/2652313002
Cr-Commit-Position: refs/heads/master@{#446300}
Committed: https://chromium.googlesource.com/chromium/src/+/f0e87360ae1a1dfae4bf0deb8cacd9db5a785466

[Benoit L, 2017-01-25 17:08:47]

lizeb@chromium.org changed reviewers:
+ droger@chromium.org

[Benoit L, 2017-01-25 17:08:56]

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

[commit-bot: I haz the power, 2017-01-25 17:09:14]

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

[droger, 2017-01-25 17:27:11]

lgtm

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_common.cc (right):

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.cc:76: return false;
Nit:
For readability it may be better to have a return statement in all the branches
or none.

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_common_unittest.cc (right):

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common_unittest.cc:135: 
Do we need a test with the feature disabled?

I think you can do something like:

base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndDisableFeature(kSpeculativeResourcePrefetchingFeatureName);

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:299:
command_line->AppendSwitchASCII("enable-features", enabled_feature);
Optional:

You may be able to do this without using the command line.

See
https://codereview.chromium.org/2537503002/diff/180001/chrome/browser/prerend...
for an example.

Not sure if it is better though.

https://codereview.chromium.org/2652313002/diff/1/tools/android/customtabs_be...
File tools/android/customtabs_benchmark/scripts/customtabs_benchmark.py (right):

https://codereview.chromium.org/2652313002/diff/1/tools/android/customtabs_be...
tools/android/customtabs_benchmark/scripts/customtabs_benchmark.py:191: #
NoStatePrefetch is enabled through an experiment.
s/NoStatePrefetch/SpeculativePrefetch/

[commit-bot: I haz the power, 2017-01-25 18:04:43]

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

[commit-bot: I haz the power, 2017-01-25 18:04:43]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-01-26 09:58:15]

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_common.cc (right):

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.cc:76: return false;
On 2017/01/25 17:27:11, droger wrote:
> Nit:
> For readability it may be better to have a return statement in all the
branches
> or none.

Done.

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_common_unittest.cc (right):

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common_unittest.cc:135: 
On 2017/01/25 17:27:11, droger wrote:
> Do we need a test with the feature disabled?
> 
> I think you can do something like:
> 
> base::test::ScopedFeatureList scoped_feature_list;
>
scoped_feature_list.InitAndDisableFeature(kSpeculativeResourcePrefetchingFeatureName);

There is 
 TEST_F(ResourcePrefetchCommonTest, IsDisabledByDefault)
above (was folded by the code review tool by default).

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2652313002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:299:
command_line->AppendSwitchASCII("enable-features", enabled_feature);
On 2017/01/25 17:27:11, droger wrote:
> Optional:
> 
> You may be able to do this without using the command line.
> 
> See
>
https://codereview.chromium.org/2537503002/diff/180001/chrome/browser/prerend...
> for an example.
> 
> Not sure if it is better though.

This works in unit tests, but not in browser tests, as the dependency is not
compiled for browser tests.
And this is actually slightly better, as we end up testing the two ways of
enabling the predictor, with the unit tests and browser tests.

https://codereview.chromium.org/2652313002/diff/1/tools/android/customtabs_be...
File tools/android/customtabs_benchmark/scripts/customtabs_benchmark.py (right):

https://codereview.chromium.org/2652313002/diff/1/tools/android/customtabs_be...
tools/android/customtabs_benchmark/scripts/customtabs_benchmark.py:191: #
NoStatePrefetch is enabled through an experiment.
On 2017/01/25 17:27:11, droger wrote:
> s/NoStatePrefetch/SpeculativePrefetch/

Thanks!

Done.

[Benoit L, 2017-01-26 09:58:20]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2017-01-26 09:58:21]

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

[commit-bot: I haz the power, 2017-01-26 09:58:35]

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-26 10:46:49]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485424700822020,
"parent_rev": "9472cff3ab42e7b07bc6dcb1ac60dfcac835683b", "commit_rev":
"f0e87360ae1a1dfae4bf0deb8cacd9db5a785466"}

[commit-bot: I haz the power, 2017-01-26 10:47:26]

Description was changed from

==========
predictors: Move the configuration of speculative prefetch to field trials.

BUG=631966
==========

to

==========
predictors: Move the configuration of speculative prefetch to field trials.

BUG=631966

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

[commit-bot: I haz the power, 2017-01-26 10:47:27]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f0e87360ae1a1dfae4bf0deb8cac...