predictors: Add a origin for prefetch requests, remove the URL/host settings.

predictors: Add a origin for prefetch requests, remove the URL/host settings.

The speculative_prefetch_predictor can start prefetching either due to
an external request, or by observing a navigation. These use cases were
not differentiated previously.
This CL adds:
- A command-line switch to enable external requests only, and
- An origin for prefetch requests.

In addition, URL and host-based learning could previously be controlled
separately. This is unused, as it was only controlled by field trial
groups, and the logic has been removed in crrev.com/2532933002.

BUG=631966
Committed: https://crrev.com/f72ac0d75ba64a1afbbaa052fb3135e1fa78fc24
Cr-Commit-Position: refs/heads/master@{#435196}

[Benoit L, 2016-11-29 12:51:25]

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

[commit-bot: I haz the power, 2016-11-29 12:51:37]

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

[Benoit L, 2016-11-29 12:54:15]

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

[commit-bot: I haz the power, 2016-11-29 13:34:24]

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

[commit-bot: I haz the power, 2016-11-29 13:34:24]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-11-29 14:44:45]

this simplifies a lot, thank you!

my suggestions are only about the hard parts - naming things, feel free to
pushback, etc.

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

https://codereview.chromium.org/2538743002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.h:70: enum Origin {
ORIGIN_NAVIGATION = 0, ORIGIN_EXTERNAL = 1 };
Please move it out of ResourcePrefetchPredictorConfig, the clients of this enum
do not have to know what the config is, right?

Reducing namespace pollution is still recommended with scoped enums, like:

enum class PrefetchOrigin {
  NAVIGATION,
  EXTERNAL
};

https://codereview.chromium.org/2538743002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.h:83: bool
IsPrefetchingEnabled(Profile* profile, Origin origin) const;
how about:

IsPrefetchingEnabledForOrigin(profile, origin),
IsPrefetchingEnabledForSomeOrigin(profile)

Otherwise the names did not clearly explain what the functions do (parameters
explain better, but I think my suggestion is even better)

Alternatively, remove the useless comment above the declaration of the 3
functions, and replace with more detailed comments for these including what they
do with |origin|.

https://codereview.chromium.org/2538743002/diff/1/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2538743002/diff/1/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:898: "enabled-external";
why not "enabled-external-only"? do you think it will confuse readers enough to
make them think this does not enable learning?

[Benoit L, 2016-11-29 17:02:51]

Thanks!
All done.

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

https://codereview.chromium.org/2538743002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.h:70: enum Origin {
ORIGIN_NAVIGATION = 0, ORIGIN_EXTERNAL = 1 };
On 2016/11/29 14:44:44, pasko wrote:
> Please move it out of ResourcePrefetchPredictorConfig, the clients of this
enum
> do not have to know what the config is, right?
> 
> Reducing namespace pollution is still recommended with scoped enums, like:
> 
> enum class PrefetchOrigin {
>   NAVIGATION,
>   EXTERNAL
> };
> 

Done.

https://codereview.chromium.org/2538743002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.h:83: bool
IsPrefetchingEnabled(Profile* profile, Origin origin) const;
On 2016/11/29 14:44:44, pasko wrote:
> how about:
> 
> IsPrefetchingEnabledForOrigin(profile, origin),
> IsPrefetchingEnabledForSomeOrigin(profile)
> 
> Otherwise the names did not clearly explain what the functions do (parameters
> explain better, but I think my suggestion is even better)
> 
> Alternatively, remove the useless comment above the declaration of the 3
> functions, and replace with more detailed comments for these including what
they
> do with |origin|.

Done.

https://codereview.chromium.org/2538743002/diff/1/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2538743002/diff/1/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:898: "enabled-external";
On 2016/11/29 14:44:44, pasko wrote:
> why not "enabled-external-only"? do you think it will confuse readers enough
to
> make them think this does not enable learning?

Done.

[Benoit L, 2016-11-29 17:03:49]

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

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

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 17:38:17]

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

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

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

[Benoit L, 2016-11-29 17:54:18]

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

[commit-bot: I haz the power, 2016-11-29 17:54:32]

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 18:41:26]

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

[commit-bot: I haz the power, 2016-11-29 18:41:27]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-11-29 19:31:30]

lgtm, thank you

[Benoit L, 2016-11-30 09:11:57]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-11-30 09:12:26]

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 09:16:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480497117184410,
"parent_rev": "12bc4a88451b8cb2631e594505b07f5cbd50c5d0", "commit_rev":
"9baa6fec8502601eda26f7fc361e1015f33c23a3"}

[commit-bot: I haz the power, 2016-11-30 09:16:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-30 09:18:42]

Description was changed from

==========
predictors: Add a origin for prefetch requests, remove the URL/host settings.

The speculative_prefetch_predictor can start prefetching either due to
an external request, or by observing a navigation. These use cases were
not differentiated previously.
This CL adds:
- A command-line switch to enable external requests only, and
- An origin for prefetch requests.

In addition, URL and host-based learning could previously be controlled
separately. This is unused, as it was only controlled by field trial
groups, and the logic has been removed in crrev.com/2532933002.

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

to

==========
predictors: Add a origin for prefetch requests, remove the URL/host settings.

The speculative_prefetch_predictor can start prefetching either due to
an external request, or by observing a navigation. These use cases were
not differentiated previously.
This CL adds:
- A command-line switch to enable external requests only, and
- An origin for prefetch requests.

In addition, URL and host-based learning could previously be controlled
separately. This is unused, as it was only controlled by field trial
groups, and the logic has been removed in crrev.com/2532933002.

BUG=631966

Committed: https://crrev.com/f72ac0d75ba64a1afbbaa052fb3135e1fa78fc24
Cr-Commit-Position: refs/heads/master@{#435196}
==========

[commit-bot: I haz the power, 2016-11-30 09:18:43]

Patchset 3 (id:??) landed as
https://crrev.com/f72ac0d75ba64a1afbbaa052fb3135e1fa78fc24
Cr-Commit-Position: refs/heads/master@{#435196}