[Prerender] Get the prerender mode from Finch field trial.

[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
TBR=pasko

Committed: https://crrev.com/bbb4856e6fbdcb6a497afd1c136e005ccf6f371d
Cr-Commit-Position: refs/heads/master@{#436932}

[droger, 2016-11-28 16:50:56]

droger@chromium.org changed reviewers:
+ mattcary@chromium.org, pasko@chromium.org

[pasko, 2016-11-29 10:51:14]

Is it going towards having a Feature, as encouraged by UMA folks? The feature
would be in the spirit of: "Replace Prerendering with a lightweight prefetching
mechanism". With that we can get comfortable tri-state of opt-in/out/experiment
in about:flags, also probably uniformly managed as other features. This would
also create less questions on launch, I think.

This change may be orthogonal to the above Feature ideal, in which case let me
know, I'll take a closer look, it just seemed contradicting at first sight.

[mattcary, 2016-11-29 11:50:39]

On 2016/11/29 10:51:14, pasko wrote:
> Is it going towards having a Feature, as encouraged by UMA folks? The feature
> would be in the spirit of: "Replace Prerendering with a lightweight
prefetching
> mechanism". With that we can get comfortable tri-state of
opt-in/out/experiment
> in about:flags, also probably uniformly managed as other features. This would
> also create less questions on launch, I think.
> 
> This change may be orthogonal to the above Feature ideal, in which case let me
> know, I'll take a closer look, it just seemed contradicting at first sight.

This CL does get the right three options: control (aka prerender as we now do
it), no-nothing (aka simple load) and no-state prefetch. If the switch isn't
given, then we do the control. I guess a slight complication is there are two
kinds of simple load states: PRERENDER_MODE_DISABLED and
PRERENDER_MODE_SIMPLE_LOAD_EXPERIMENT, the difference being that in the simple
load experiment we track the prefetching histograms correctly, and in the
disabled state we track the prefetching histograms incorrectly.

This suggests that we should drop the PRERENDER_MODE_DISABLED option, as I don't
see a situation where that would be a valid operating state, except for the
peace of mind that a kill-switch gives us.

[droger, 2016-11-29 12:44:37]

Description was changed from

==========
[Prerender] Get the prerender mode from Finch field trial.

BUG=668997
==========

to

==========
[Prerender] Get the prerender mode from Finch field trial.

BUG=668997
==========

[droger, 2016-11-29 12:44:37]

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

[droger, 2016-11-29 12:45:43]

On 2016/11/29 11:50:39, mattcary wrote:
> On 2016/11/29 10:51:14, pasko wrote:
> > Is it going towards having a Feature, as encouraged by UMA folks? The
feature
> > would be in the spirit of: "Replace Prerendering with a lightweight
> prefetching
> > mechanism". With that we can get comfortable tri-state of
> opt-in/out/experiment
> > in about:flags, also probably uniformly managed as other features. This
would
> > also create less questions on launch, I think.
> > 
> > This change may be orthogonal to the above Feature ideal, in which case let
me
> > know, I'll take a closer look, it just seemed contradicting at first sight.
> 
> This CL does get the right three options: control (aka prerender as we now do
> it), no-nothing (aka simple load) and no-state prefetch. If the switch isn't
> given, then we do the control. I guess a slight complication is there are two
> kinds of simple load states: PRERENDER_MODE_DISABLED and
> PRERENDER_MODE_SIMPLE_LOAD_EXPERIMENT, the difference being that in the simple
> load experiment we track the prefetching histograms correctly, and in the
> disabled state we track the prefetching histograms incorrectly.
> 
> This suggests that we should drop the PRERENDER_MODE_DISABLED option, as I
don't
> see a situation where that would be a valid operating state, except for the
> peace of mind that a kill-switch gives us.

Yes, sorry, I'm reworking this CL to use the feature API, so you can hold on the
review for now.

I'm still not convinced this is really possible to do though, because we need
feature parameters, and it seems that they are not supported in the client.
I see that there is some support for parameters through the variations
component:
https://cs.chromium.org/chromium/src/components/variations/variations_associa...
but in particular the support for command line arguments seems to be missing.

The design doc says:
"Feature parameters:
Once we have a top-level concept of Feature, I suspect we will eventually get
requests for feature parameters, similar to Finch parameters. I am intentionally
leaving out the design of this from the initial Feature API design - to make the
v0 of Feature API more tractable. However, I don't think there's anything in
this design that will prevent extending it to support params."

rkaplow: do you have any recommendation on how to pass feature arguments from
the command line?

[droger, 2016-11-29 13:58:13]

Looking more at docs and code I found maybe a couple ways to do it.

One is documented here:
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/experiment-params.md?c...

The command line syntax is a bit ugly and basically requires passing a Finch
config on the command line.


Another option may be to leverage the flag_ui component, although it's not clear
exactly how that works.

[mattcary, 2016-11-29 23:00:03]

On 2016/11/29 13:58:13, droger wrote:
> Looking more at docs and code I found maybe a couple ways to do it.
> 
> One is documented here:
>
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/experiment-params.md?c...
> 
> The command line syntax is a bit ugly and basically requires passing a Finch
> config on the command line.
> 
> 
> Another option may be to leverage the flag_ui component, although it's not
clear
> exactly how that works.

Copy/pasting a complicated command line isn't a big deal if the finch folks are
much happier with us using the new system.

It sounds like that might be a simple, reliable way to do it. We don't need to
over think it.

[droger, 2016-11-30 16:49:14]

Patchset #3 (id:40001) has been deleted

[droger, 2016-11-30 16:49:23]

Patchset #3 (id:60001) has been deleted

[droger, 2016-11-30 16:50:46]

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

[commit-bot: I haz the power, 2016-11-30 16:51:27]

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

[droger, 2016-11-30 17:05:26]

Updated the CL to use the feature API.

The feature is called "NoStatePrefetch", but I can change it to be "Prerender".

It can be disabled (similar to --prerender=disabled) or enabled with one of the
3 modes: prerender, no_state_prefetch, simple_load.

The CL removes the existing command line flags, since they conflict with the
Feature code (Feature intends to take over the command line parameters).

[droger, 2016-11-30 17:07:27]

On 2016/11/30 17:05:26, droger wrote:
> Updated the CL to use the feature API.
> 
> The feature is called "NoStatePrefetch", but I can change it to be
"Prerender".
> 
> It can be disabled (similar to --prerender=disabled) or enabled with one of
the
> 3 modes: prerender, no_state_prefetch, simple_load.
> 
> The CL removes the existing command line flags, since they conflict with the
> Feature code (Feature intends to take over the command line parameters).

Forgot to add:
unless we find something more convenient, forcing the prerender mode requires a
somewhat complex command line, like:

chrome --force-fieldtrials=NoStatePrefetchValidation/Prerender
--enable-features="NoStatePrefetch"
--force-fieldtrial-params=NoStatePrefetchValidation.Prerender:mode/no_state_prefetch

[droger, 2016-11-30 17:09:09]

On 2016/11/30 17:07:27, droger wrote:
> On 2016/11/30 17:05:26, droger wrote:
> > Updated the CL to use the feature API.
> > 
> > The feature is called "NoStatePrefetch", but I can change it to be
> "Prerender".
> > 
> > It can be disabled (similar to --prerender=disabled) or enabled with one of
> the
> > 3 modes: prerender, no_state_prefetch, simple_load.
> > 
> > The CL removes the existing command line flags, since they conflict with the
> > Feature code (Feature intends to take over the command line parameters).
> 
> Forgot to add:
> unless we find something more convenient, forcing the prerender mode requires
a
> somewhat complex command line, like:
> 
> chrome --force-fieldtrials=NoStatePrefetchValidation/Prerender
> --enable-features="NoStatePrefetch"
>
--force-fieldtrial-params=NoStatePrefetchValidation.Prerender:mode/no_state_prefetch

Argh, missing a small bit. It should be:

chrome --force-fieldtrials=NoStatePrefetchValidation/Prerender
--enable-features="NoStatePrefetch<NoStatePrefetchValidation"
--force-fieldtrial-params=NoStatePrefetchValidation.Prerender:mode/no_state_prefetch

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

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

[commit-bot: I haz the power, 2016-11-30 18:23:10]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-12-01 16:47:58]

a few structural+testing suggestions, otherwise looks good, nice cleanup as
well, thanks!

toplevel: should we document somewhere other than in this discussion, how to set
all possible params for prerender/prefetch? A comment in code would work for me.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (left):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:39:
command_line->AppendSwitchASCII(switches::kPrerenderMode,
if we are removing this, then the tests will need to be adjusted when we turn
off prerender by default, right? is there a way to force the feature state for
the test? I guess that would make switching things on/off reverting/relanding
easier.

If forcing the feature into certain state is hard here, then np.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:18:
base::FEATURE_ENABLED_BY_DEFAULT};
maybe disabled by default?

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.h (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.h:14: extern const base::Feature
kNoStatePrefetchFeature;
do we want the feature in about:flags as well? (sounds handy to me)

otherwise, this extern const is used only for testing, so it would be preferable
to explicitly say so:

extern const& base::Feature GetNoStatePrefetchFeatureForTesting()

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:111:
PrerenderManager::SetMode(PrerenderManager::PRERENDER_MODE_NOSTATE_PREFETCH);
that's very repetitive, can we do it in c-tor or SetUp?

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:417:
scoped_feature_list.InitAndDisableFeature(kNoStatePrefetchFeature);
please also test setting the "simple_load"

[droger, 2016-12-01 17:17:41]

Thanks.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (left):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:39:
command_line->AppendSwitchASCII(switches::kPrerenderMode,
On 2016/12/01 16:47:58, pasko wrote:
> if we are removing this, then the tests will need to be adjusted when we turn
> off prerender by default, right? is there a way to force the feature state for
> the test? I guess that would make switching things on/off reverting/relanding
> easier.
> 
> If forcing the feature into certain state is hard here, then np.

I don't have a simple solution unfortunately.
We could still do it by appending command line arguments though, they would just
be more complicated:

--force-fieldtrials=NoStatePrefetchValidation/Prerender
--enable-features="NoStatePrefetch<NoStatePrefetchValidation"
--force-fieldtrial-params=NoStatePrefetchValidation.Prerender:mode/no_state_prefetch

We can probably have a helper function to do this if needed.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:18:
base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/12/01 16:47:58, pasko wrote:
> maybe disabled by default?

No, it is enabled by default (but in "Prerender" mode).

The feature disabled means that prerender is disabled.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.h (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.h:14: extern const base::Feature
kNoStatePrefetchFeature;
On 2016/12/01 16:47:58, pasko wrote:
> do we want the feature in about:flags as well? (sounds handy to me)
> 
> otherwise, this extern const is used only for testing, so it would be
preferable
> to explicitly say so:
> 
> extern const& base::Feature GetNoStatePrefetchFeatureForTesting()

It seems that there is one (and only one) feature that uses about_flags in
combination with finch parameters (i.e. FEATURE_WITH_VARIATIONS_VALUE_TYPE)
which is ntp_snippets.

I can look into it and see if that works for us.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:111:
PrerenderManager::SetMode(PrerenderManager::PRERENDER_MODE_NOSTATE_PREFETCH);
On 2016/12/01 16:47:58, pasko wrote:
> that's very repetitive, can we do it in c-tor or SetUp?

No.

One option would be to keep the SetUpCommandLine() method, and use the finch
flags there.

[droger, 2016-12-01 17:19:35]

Description was changed from

==========
[Prerender] Get the prerender mode from Finch field trial.

BUG=668997
==========

to

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
==========

[pasko, 2016-12-01 17:30:44]

only one answer, have to run to a meeting :)

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:18:
base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/12/01 17:17:40, droger wrote:
> On 2016/12/01 16:47:58, pasko wrote:
> > maybe disabled by default?
> 
> No, it is enabled by default (but in "Prerender" mode).
> 
> The feature disabled means that prerender is disabled.

I think the general ideology of a Feature is to have it disabled when the
feature is in development, then enable it (via finch, then permanently), then
remove the mention of the feature potentially. So starting as enabled looks
counter-intuitive to me. Also, feature "NoStatePrefetch" to mean "Prerender"
when enabled may confuse people, no?

[droger, 2016-12-01 17:39:49]

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:18:
base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/12/01 17:30:44, pasko wrote:
> On 2016/12/01 17:17:40, droger wrote:
> > On 2016/12/01 16:47:58, pasko wrote:
> > > maybe disabled by default?
> > 
> > No, it is enabled by default (but in "Prerender" mode).
> > 
> > The feature disabled means that prerender is disabled.
> 
> I think the general ideology of a Feature is to have it disabled when the
> feature is in development, then enable it (via finch, then permanently), then
> remove the mention of the feature potentially. So starting as enabled looks
> counter-intuitive to me. Also, feature "NoStatePrefetch" to mean "Prerender"
> when enabled may confuse people, no?

I think it would be even more confusing, but that could work too.

In that world, we would have:

feature disabled = prerender

feature enabled with 3 modes:
- simple load
- prefetch
- disabled

Having a feature enabled with a disabled mode seems strange to me...

Or maybe we could have a feature + another separate switch to disable
everything?

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:111:
PrerenderManager::SetMode(PrerenderManager::PRERENDER_MODE_NOSTATE_PREFETCH);
On 2016/12/01 17:17:41, droger wrote:
> On 2016/12/01 16:47:58, pasko wrote:
> > that's very repetitive, can we do it in c-tor or SetUp?
> 
> No.
> 
> One option would be to keep the SetUpCommandLine() method, and use the finch
> flags there.

Maybe I spoke too soon, as SetupOnMainThread() seems to work.

[droger, 2016-12-01 17:42:03]

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

[commit-bot: I haz the power, 2016-12-01 17:42:28]

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

[droger, 2016-12-01 17:48:27]

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:18:
base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/12/01 17:39:49, droger wrote:
> On 2016/12/01 17:30:44, pasko wrote:
> > On 2016/12/01 17:17:40, droger wrote:
> > > On 2016/12/01 16:47:58, pasko wrote:
> > > > maybe disabled by default?
> > > 
> > > No, it is enabled by default (but in "Prerender" mode).
> > > 
> > > The feature disabled means that prerender is disabled.
> > 
> > I think the general ideology of a Feature is to have it disabled when the
> > feature is in development, then enable it (via finch, then permanently),
then
> > remove the mention of the feature potentially. So starting as enabled looks
> > counter-intuitive to me. Also, feature "NoStatePrefetch" to mean "Prerender"
> > when enabled may confuse people, no?
> 
> I think it would be even more confusing, but that could work too.
> 
> In that world, we would have:
> 
> feature disabled = prerender
> 
> feature enabled with 3 modes:
> - simple load
> - prefetch
> - disabled
> 
> Having a feature enabled with a disabled mode seems strange to me...
> 
> Or maybe we could have a feature + another separate switch to disable
> everything?

Also in my mind, in the long term we would not remove the feature, but keep one
feature called NoStatePrefetch that can be enabled and disabled. We would remove
the finch experiment and the parameters though.
And my code is kind of a first step towards that.

[commit-bot: I haz the power, 2016-12-01 18:24:37]

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

[commit-bot: I haz the power, 2016-12-01 18:24:38]

Dry run: This issue passed the CQ dry run.

[droger, 2016-12-02 16:51:23]

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_field_trial.h (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.h:14: extern const base::Feature
kNoStatePrefetchFeature;
On 2016/12/01 17:17:40, droger wrote:
> On 2016/12/01 16:47:58, pasko wrote:
> > do we want the feature in about:flags as well? (sounds handy to me)
> > 
> > otherwise, this extern const is used only for testing, so it would be
> preferable
> > to explicitly say so:
> > 
> > extern const& base::Feature GetNoStatePrefetchFeatureForTesting()
> 
> It seems that there is one (and only one) feature that uses about_flags in
> combination with finch parameters (i.e. FEATURE_WITH_VARIATIONS_VALUE_TYPE)
> which is ntp_snippets.
> 
> I can look into it and see if that works for us.

I looked into this, and I have a working prototype. That would better be done in
another CL though.

Note that adding the feature to about://flags does not allow to set its value
from the command line either :(

[mattcary, 2016-12-05 14:07:14]

lgtm

https://codereview.chromium.org/2537503002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_field_trial.cc:27: kNoStatePrefetchFeature,
"mode");
Wouldn't we prefer "mode" and the mode types to be constants? Particularly in
this case it would make it a lot easier to have a summary of what the possible
modes are rather than digging through if stmts.

[droger, 2016-12-05 14:44:56]

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

[commit-bot: I haz the power, 2016-12-05 14:45:09]

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

[droger, 2016-12-05 14:48:27]

pasko: Everything addressed, the only outstanding issue is the semantics of the
Feature (what does it means to be enabled, and what are the parameters).

Maybe we should meet to discuss this when you have time, that would be simpler.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (left):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:39:
command_line->AppendSwitchASCII(switches::kPrerenderMode,
On 2016/12/01 16:47:58, pasko wrote:
> if we are removing this, then the tests will need to be adjusted when we turn
> off prerender by default, right? is there a way to force the feature state for
> the test? I guess that would make switching things on/off reverting/relanding
> easier.
> 
> If forcing the feature into certain state is hard here, then np.

Done.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:417:
scoped_feature_list.InitAndDisableFeature(kNoStatePrefetchFeature);
On 2016/12/01 16:47:58, pasko wrote:
> please also test setting the "simple_load"

Done.

[droger, 2016-12-05 15:05:21]

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

[commit-bot: I haz the power, 2016-12-05 15:05:36]

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

[droger, 2016-12-05 15:05:43]

Patchset #6 (id:140001) has been deleted

[droger, 2016-12-05 15:05:51]

https://codereview.chromium.org/2537503002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/2537503002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_field_trial.cc:27: kNoStatePrefetchFeature,
"mode");
On 2016/12/05 14:07:13, mattcary wrote:
> Wouldn't we prefer "mode" and the mode types to be constants? Particularly in
> this case it would make it a lot easier to have a summary of what the possible
> modes are rather than digging through if stmts.

Done.

[commit-bot: I haz the power, 2016-12-05 15:53:17]

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

[commit-bot: I haz the power, 2016-12-05 15:53:18]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2016-12-05 18:27:08]

lgtm

From the metrics side this lgtm

[droger, 2016-12-07 12:41:05]

Description was changed from

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
==========

to

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
TBR=pasko
==========

[droger, 2016-12-07 12:41:35]

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

[droger, 2016-12-07 12:50:53]

TBR pasko, as per offline discussion.

TBR jochen for simple changes in:
- app_url_redirector_browsertest
- net_internals_ui_browsertest
- activity_log_browsertest
Callsite updated (removed parameter) in:
- chrome/browser/chrome_browser_main.cc

[droger, 2016-12-07 12:53:33]

The CQ bit was checked by droger@chromium.org

[droger, 2016-12-07 12:53:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2537503002/#ps180001
(title: "cleanup")

[commit-bot: I haz the power, 2016-12-07 12:53:52]

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-07 13:35:37]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481115213370550,
"parent_rev": "1a36b17bee7cda17efe77b0607f4b3350a3daa89", "commit_rev":
"ba62fe44b9999a97181a5a5ab6f12cb427e4af85"}

[commit-bot: I haz the power, 2016-12-07 13:36:09]

Description was changed from

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
TBR=pasko
==========

to

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
TBR=pasko
==========

[commit-bot: I haz the power, 2016-12-07 13:36:10]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-12-07 13:38:02]

Description was changed from

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
TBR=pasko
==========

to

==========
[Prerender] Get the prerender mode from Finch field trial.

This CL introduces a new NoStatePrefetch feature, controlled by Finch.

The feature can be disabled, and then there is no prerender or prefetch
happening.

It can be enabled with 3 modes:
- prerender (default for now)
- prefetch
- simple load.

BUG=668997
TBR=pasko

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

[commit-bot: I haz the power, 2016-12-07 13:38:03]

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

[pasko, 2016-12-08 15:23:44]

lgtm

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (left):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:39:
command_line->AppendSwitchASCII(switches::kPrerenderMode,
On 2016/12/01 17:17:40, droger wrote:
> On 2016/12/01 16:47:58, pasko wrote:
> > if we are removing this, then the tests will need to be adjusted when we
turn
> > off prerender by default, right? is there a way to force the feature state
for
> > the test? I guess that would make switching things on/off
reverting/relanding
> > easier.
> > 
> > If forcing the feature into certain state is hard here, then np.
> 
> I don't have a simple solution unfortunately.
> We could still do it by appending command line arguments though, they would
just
> be more complicated:
> 
> --force-fieldtrials=NoStatePrefetchValidation/Prerender
> --enable-features="NoStatePrefetch<NoStatePrefetchValidation"
>
--force-fieldtrial-params=NoStatePrefetchValidation.Prerender:mode/no_state_prefetch
> 
> We can probably have a helper function to do this if needed.

In the last version of the code, the above instructions would not work, or would
they? I just could not find where it is documented in the end. Let's put the
howto on enabling/disabling the feature from command-line in the design doc? In
the code would also be OK, but needs upload/review/commit overhead.

[droger, 2016-12-08 15:47:30]

Thanks for the review.

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (left):

https://codereview.chromium.org/2537503002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:39:
command_line->AppendSwitchASCII(switches::kPrerenderMode,
On 2016/12/08 15:23:44, pasko wrote:
> On 2016/12/01 17:17:40, droger wrote:
> > On 2016/12/01 16:47:58, pasko wrote:
> > > if we are removing this, then the tests will need to be adjusted when we
> turn
> > > off prerender by default, right? is there a way to force the feature state
> for
> > > the test? I guess that would make switching things on/off
> reverting/relanding
> > > easier.
> > > 
> > > If forcing the feature into certain state is hard here, then np.
> > 
> > I don't have a simple solution unfortunately.
> > We could still do it by appending command line arguments though, they would
> just
> > be more complicated:
> > 
> > --force-fieldtrials=NoStatePrefetchValidation/Prerender
> > --enable-features="NoStatePrefetch<NoStatePrefetchValidation"
> >
>
--force-fieldtrial-params=NoStatePrefetchValidation.Prerender:mode/no_state_prefetch
> > 
> > We can probably have a helper function to do this if needed.
> 
> In the last version of the code, the above instructions would not work, or
would
> they? I just could not find where it is documented in the end. Let's put the
> howto on enabling/disabling the feature from command-line in the design doc?
In
> the code would also be OK, but needs upload/review/commit overhead.

I think they would work, but I did not need it in this CL.
Instead I called directly prerender::PrerenderManager::SetMode() from the test
setup.

This command line is still needed to set the prerender mode from the command
line.
Another option is to use about://flags.

I'll document this.

[pasko, 2016-12-09 14:52:48]

On 2016/12/08 15:47:30, droger wrote:
> I think they would work, but I did not need it in this CL.
> Instead I called directly prerender::PrerenderManager::SetMode() from the test
> setup.
> 
> This command line is still needed to set the prerender mode from the command
> line.
> Another option is to use about://flags.
> 
> I'll document this.

Thanks! Documentation would really help.

[pasko, 2016-12-09 14:52:49]

On 2016/12/08 15:47:30, droger wrote:
> I think they would work, but I did not need it in this CL.
> Instead I called directly prerender::PrerenderManager::SetMode() from the test
> setup.
> 
> This command line is still needed to set the prerender mode from the command
> line.
> Another option is to use about://flags.
> 
> I'll document this.

Thanks! Documentation would really help.

[droger, 2016-12-09 15:29:00]

On 2016/12/09 14:52:49, pasko wrote:
> Thanks! Documentation would really help.

I added a section in the design document: "Manually setting the prefetch mode"
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5E...