Prerender: omnibox and instant field trial parameters.

Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

BUG=565501, 678332
TBR=vabr@chromium.org

Review-Url: https://codereview.chromium.org/2754003006
Cr-Commit-Position: refs/heads/master@{#458124}
Committed: https://chromium.googlesource.com/chromium/src/+/6430db2fde6019843ea53e3540757857934b804a

[mattcary, 2017-03-17 15:50:02]

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

[mattcary, 2017-03-17 15:50:03]

Here's a proposal for the omnibox/instant flag organization. A summary of the
choices of made is:

* control omnibox and instant separately.

* default them to whatever is set for the overall experiment if they aren't
explicitly specified. This means that nothing should change for the current
experiment, but that we have to be careful about not turning them off.

* I updated existing uses of the various PrerenderEnabled to be accurate.
Hopefully.

[pasko, 2017-03-17 16:38:32]

looking at it, perhaps the BUG should point at both launch bugs?

[pasko, 2017-03-17 17:22:54]

also: dry run?

[pasko, 2017-03-17 17:23:01]

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

[commit-bot: I haz the power, 2017-03-17 17:23:51]

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

[pasko, 2017-03-17 17:33:33]

lgtm, my comments are mostly nits (but please nevertheless answer them)

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:23: // Overriden prerender
mode origins.
maybe s/origins/keys/ would make it more obvious that the values below are for
these keys? Maybe a bit more explanation how it works on the top?

really a nit, hopefully this code does not survive for long.

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:36:
PrerenderManager::PrerenderManagerMode ParsePrerenderMode(
if the function is not used outside this file, it should be in the anonymous
namespace

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:43: std::string mode_value =
base::GetFieldTrialParamValueByFeature(
did you change the function solely because of deprecation of the old?

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.h:16: extern const char
kNoStatePrefetchFeatureInstantModeParameterName[];
nit: maybe add a comment that these are exposed outside the .cc for testing
only? I'd actually prefer to duplicate the strings in the tests, but it's better
be done later.

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:429: ASSERT_TRUE(
this would return from the function if the condition does not hold, do we really
want it? do we care?

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:441:
EXPECT_EQ(base::FeatureList::GetFieldTrial(kNoStatePrefetchFeature), trial);
This is a utility function, not a test, I think it would be more readable to
perform these EXPECT_* checks in one of the tests
(PrerenderRespectsFieldTrialParameters?) and assume it always works.

[commit-bot: I haz the power, 2017-03-17 17:36:33]

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

[commit-bot: I haz the power, 2017-03-17 17:36:34]

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

[pasko, 2017-03-17 18:32:10]

Please also update the description to make it clearer for people not familiar
with prerender internals.

Specifically:
* explicitly mention what parameters are added to the NoStatePrefetch
base::Feature
* the list of strings supported as values
* how to force these modes via commandline (like in the nosate doc:
http://goo.gl/EJjTCM - the doc should probably be updated later to cover this
separation, right?)

[mattcary, 2017-03-20 10:12:35]

On 2017/03/17 16:38:32, pasko wrote:
> looking at it, perhaps the BUG should point at both launch bugs?

Sure, added 565501 and 678332.

[mattcary, 2017-03-20 10:12:46]

Description was changed from

==========
Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

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

to

==========
Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

BUG=565501,678332
==========

[mattcary, 2017-03-20 10:20:32]

Looking into the various dry run failures now as everyone seems to be happy with
the new parameter setup.

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:23: // Overriden prerender
mode origins.
On 2017/03/17 17:33:33, pasko wrote:
> maybe s/origins/keys/ would make it more obvious that the values below are for
> these keys? Maybe a bit more explanation how it works on the top?
> 
> really a nit, hopefully this code does not survive for long.

Done.

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:36:
PrerenderManager::PrerenderManagerMode ParsePrerenderMode(
On 2017/03/17 17:33:33, pasko wrote:
> if the function is not used outside this file, it should be in the anonymous
> namespace

Done.

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.cc:43: std::string mode_value =
base::GetFieldTrialParamValueByFeature(
On 2017/03/17 17:33:33, pasko wrote:
> did you change the function solely because of deprecation of the old?

Yup.

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_field_trial.h:16: extern const char
kNoStatePrefetchFeatureInstantModeParameterName[];
On 2017/03/17 17:33:33, pasko wrote:
> nit: maybe add a comment that these are exposed outside the .cc for testing
> only? I'd actually prefer to duplicate the strings in the tests, but it's
better
> be done later.

Done.

I go both ways on duplicating strings in the tests.

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:429: ASSERT_TRUE(
On 2017/03/17 17:33:33, pasko wrote:
> this would return from the function if the condition does not hold, do we
really
> want it? do we care?

This returns false if, for example, one tries to associate multiple params with
a single trial group. This is why I had to add the tearDown() method. It was a
lot easier seeing this assert fail than to have the params for a test not get
applied and have a tricky test failure to debug.

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:441:
EXPECT_EQ(base::FeatureList::GetFieldTrial(kNoStatePrefetchFeature), trial);
On 2017/03/17 17:33:33, pasko wrote:
> This is a utility function, not a test, I think it would be more readable to
> perform these EXPECT_* checks in one of the tests
> (PrerenderRespectsFieldTrialParameters?) and assume it always works.

Changed to an assert, see previous comment.

[mattcary, 2017-03-20 12:21:57]

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

[commit-bot: I haz the power, 2017-03-20 12: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-03-20 13:10:34]

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

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

Dry run: This issue passed the CQ dry run.

[mattcary, 2017-03-20 13:16:28]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2017-03-20 13:16:29]

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

[commit-bot: I haz the power, 2017-03-20 13:16:34]

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-20 13:22:47]

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

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

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

[pasko, 2017-03-20 13:24:55]

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

https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:429: ASSERT_TRUE(
On 2017/03/20 10:20:31, mattcary wrote:
> On 2017/03/17 17:33:33, pasko wrote:
> > this would return from the function if the condition does not hold, do we
> really
> > want it? do we care?
> 
> This returns false if, for example, one tries to associate multiple params
with
> a single trial group. This is why I had to add the tearDown() method. It was a
> lot easier seeing this assert fail than to have the params for a test not get
> applied and have a tricky test failure to debug.

Let me rephrase: ASSERT_* returns from the subroutine, we should use
ASSERT_NO_FATAL_FAILURE with them if we want exception-like propagation of
assertions in tests.

See:
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

[mattcary, 2017-03-20 13:28:59]

On 2017/03/20 13:24:55, pasko wrote:
>
https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_unittest.cc (right):
> 
>
https://codereview.chromium.org/2754003006/diff/40001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_unittest.cc:429: ASSERT_TRUE(
> On 2017/03/20 10:20:31, mattcary wrote:
> > On 2017/03/17 17:33:33, pasko wrote:
> > > this would return from the function if the condition does not hold, do we
> > really
> > > want it? do we care?
> > 
> > This returns false if, for example, one tries to associate multiple params
> with
> > a single trial group. This is why I had to add the tearDown() method. It was
a
> > lot easier seeing this assert fail than to have the params for a test not
get
> > applied and have a tricky test failure to debug.
> 
> Let me rephrase: ASSERT_* returns from the subroutine, we should use
> ASSERT_NO_FATAL_FAILURE with them if we want exception-like propagation of
> assertions in tests.
> 
> See:
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

Ah, so my original EXPECT_* was better until your comment suggested a change? ;)
 Sounds like I should have just stuck with that.

[droger, 2017-03-20 13:30:51]

lgtm, sorry for the delay

[mattcary, 2017-03-20 17:59:16]

Description was changed from

==========
Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

BUG=565501,678332
==========

to

==========
Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

BUG=565501,678332
TBR=vabr@chromium.org
==========

[mattcary, 2017-03-20 17:59:16]

mattcary@chromium.org changed reviewers:
+ vabr@chromium.org

[mattcary, 2017-03-20 18:00:07]

vabr@ as TBR for API affecting login_handler_browsertest.cc.

[mattcary, 2017-03-20 18:01:19]

The CQ bit was checked by mattcary@chromium.org

[commit-bot: I haz the power, 2017-03-20 18:01:55]

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-20 18:16:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490032879265690,
"parent_rev": "bb38b3e1e8d6ebfb2d2fcedcd1acb5be70f65cd9", "commit_rev":
"6430db2fde6019843ea53e3540757857934b804a"}

[commit-bot: I haz the power, 2017-03-20 18:16:56]

Description was changed from

==========
Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

BUG=565501,678332
TBR=vabr@chromium.org
==========

to

==========
Prerender: omnibox and instant field trial parameters.

Allow omnibox and instant prerendering origins to be controlled separately from
the rest of the NoStatePrefetch experiment.

BUG=565501,678332
TBR=vabr@chromium.org

Review-Url: https://codereview.chromium.org/2754003006
Cr-Commit-Position: refs/heads/master@{#458124}
Committed:
https://chromium.googlesource.com/chromium/src/+/6430db2fde6019843ea53e354075...
==========

[commit-bot: I haz the power, 2017-03-20 18:16:57]

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

[vabr (Chromium), 2017-03-20 18:19:21]

On 2017/03/20 18:00:07, mattcary wrote:
> vabr@ as TBR for API affecting login_handler_browsertest.cc.

LGTM, thanks!
Cheers,
Vaclav

[pasko, 2017-03-20 19:42:16]

On 2017/03/17 18:32:10, pasko wrote:
> Please also update the description to make it clearer for people not familiar
> with prerender internals.
> 
> Specifically:
> * explicitly mention what parameters are added to the NoStatePrefetch
> base::Feature
> * the list of strings supported as values
> * how to force these modes via commandline (like in the nosate doc:
> http://goo.gl/EJjTCM - the doc should probably be updated later to cover this
> separation, right?)

This slipped unnoticed and now there is no way to fix it :(

I will attempt to hold my future l-g-t-m-s until I am satisfied with commit
description.