Omnibox: Allow Bundled Omnibox Field Trial to Examine Instant Extended

Omnibox: Allow Bundled Omnibox Field Trial to Examine Instant Extended

Make the experiments in the bundled omnibox field trial be able to 
specify whether they should on for instant-extended-enabled users, off 
for those users but on for everyone else, or on for everyone.

I tested this interactively using a local variations server.  Plus
there's improved unit tests.

BUG=267584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216848

[Mark P, 2013-08-08 16:08:38]

Hi Peter,

This isn't an important/pressing as the other pending changes, but, regardless,
it's ready for review.  This should be pretty easy to review.  (It's small,
comparatively.)  It may require some resolving depending on which other changes
go in first, but those changes will be straightforward.

--mark

[Peter Kasting, 2013-08-09 20:45:44]

LGTM

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.cc:274: //  
key=<Rule>:<AutocompleteInput::PageClassification (as an int)>:<whether
Nit: Consider breaking after each colon for readability (and maybe noting there
are no linebreaks in the real rules)

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.cc:279: // context.
Nit: regardless of relevant context -> for all values of the matching portion of
the context?

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.cc:315: // Backoff to the global page
classification context.
Nit: Backoff -> Fall back (2 places)

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.h:153: // current context, backoff to
the rule in various wildcard contexts and
Nit; backoff to the rule -> fall back to any rules ... return their values ?

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.h:155: // context, returns the empty
string.  For more details, see the
Nit: Perhaps "For more details, including how we prioritize different wildcard
contexts, ..."?

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right):

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:208: // Tests for Instant
Extended is off.
Nit: "Tests with Instant Extended disabled."?

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:212:
OmniboxFieldTrial::GetValueForRuleInContext(
Nit: It might be possible to condense this test case and improve its readability
by writing a shorter-named wrapper for this function (and moving the EOL
comments up a line).  Not sure.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:213: "rule1",
AutocompleteInput::NEW_TAB_PAGE));  // exact match
Nit: In principle, it would be nice if all these EOL comments aligned.

[Mark P, 2013-08-09 21:55:01]

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.cc:274: //  
key=<Rule>:<AutocompleteInput::PageClassification (as an int)>:<whether
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: Consider breaking after each colon for readability (and maybe noting
there
> are no linebreaks in the real rules)

Nice idea: it makes the elements more readable and makes it easier to add them
in the future.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.cc:279: // context.
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: regardless of relevant context -> for all values of the matching portion
of
> the context?

Sure.  I spent some time trying to choose these words and ended up with
"regardless".  Your phrasing is better.  Replaced.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.cc:315: // Backoff to the global page
classification context.
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: Backoff -> Fall back (2 places)

Done.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.h:153: // current context, backoff to
the rule in various wildcard contexts and
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit; backoff to the rule -> fall back to any rules ... return their values ?

Changed backoff to fall back.  Left the singular rule because rule is specific
(SearchHistory, ReorderForInlining, etc.), and it doesn't fall back to any rule,
it falls back to that rule in different contexts.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial.h:155: // context, returns the empty
string.  For more details, see the
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: Perhaps "For more details, including how we prioritize different wildcard
> contexts, ..."?

Done.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right):

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:208: // Tests for Instant
Extended is off.
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: "Tests with Instant Extended disabled."?

Done.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:212:
OmniboxFieldTrial::GetValueForRuleInContext(
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: It might be possible to condense this test case and improve its
readability
> by writing a shorter-named wrapper for this function (and moving the EOL
> comments up a line).  Not sure.

I wasn't sure either, but I went and tried it and like how it looks.

https://codereview.chromium.org/22698002/diff/3001/chrome/browser/omnibox/omn...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:213: "rule1",
AutocompleteInput::NEW_TAB_PAGE));  // exact match
On 2013/08/09 20:45:45, Peter Kasting wrote:
> Nit: In principle, it would be nice if all these EOL comments aligned.

Done.

[commit-bot: I haz the power, 2013-08-09 22:01:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/22698002/10001

[commit-bot: I haz the power, 2013-08-10 05:34:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/22698002/32001

[commit-bot: I haz the power, 2013-08-10 14:09:04]

Change committed as 216848