Omnibox: Convert Shortcuts Scoring Field Trial to Omnibox Bundled Field Trial

Omnibox: Convert Shortcuts Scoring Field Trial to Omnibox Bundled Field Trial

Switch from using its own field trial to being part of the bundled one.

As a natural consequence of this change, we can not alter the shortcuts
max relevance based on context in which the user is using the omnibox.

Tested interactively using a local variations server.

Also, adds missing inputs in certain files while I'm at it.

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

[Mark P, 2013-08-09 23:27:15]

Peter,

I bet you that you thought you were done with my changes.  Hah!  :-P

This one is straightforward.

I'll need to resolve it slightly once the change goes in that makes the omnibox
bundled experiment rule names public, but I think you can review it now and
trust me to fix that when I need to.

This really will be the last review for a while, I promise.  I'm pretty burned
out on chrome coding for a bit; I think I'll focus on data/metrics/analysis
stuff for the next week or few as I launch experiments.

thanks,
mark

P.S.  Note that this is targeting M-30, but I'm fine with you reviewing this on
Monday; I am not expecting difficulty with landing this manually on Monday.

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:14: #include
"chrome/browser/autocomplete/autocomplete_input.h"
This change doesn't need it, but this should've been here all along.

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right):

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial_unittest.cc:11: #include
"chrome/browser/autocomplete/autocomplete_input.h"
This isn't needed for this change but I noticed it should've been here all
along.

[Peter Kasting, 2013-08-09 23:33:15]

LGTM

https://codereview.chromium.org/22354006/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/22354006/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/shortcuts_provider.cc:157: if
(!OmniboxFieldTrial::ShortcutsScoringMaxRelevance(
Nit: Perhaps we should default-initialize the temp, and simply make this
function not touch its argument on failure.  Then we need not even have a return
value.

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:213: // The value of the rule is
an integer displayed as a string that
Nit: "...is a string that encodes an integer containing the max relevance"?

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:221: LOG(WARNING) << "Malformed
MaxRelevance string: " << max_relevance_str;
While here, consider removing this LOG unless you have plans for how you will
gather the logging data and make use of it.

[Mark P, 2013-08-10 00:11:56]

https://codereview.chromium.org/22354006/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/22354006/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/shortcuts_provider.cc:157: if
(!OmniboxFieldTrial::ShortcutsScoringMaxRelevance(
On 2013/08/09 23:33:15, Peter Kasting wrote:
> Nit: Perhaps we should default-initialize the temp, and simply make this
> function not touch its argument on failure.  Then we need not even have a
return
> value.

I prefer this type of structure (success_bool, variable_to_set).  I've run into
too many annoying interfaces that make it difficult to tell the difference
between failure and success but empty or success but no need to modify.

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:213: // The value of the rule is
an integer displayed as a string that
On 2013/08/09 23:33:15, Peter Kasting wrote:
> Nit: "...is a string that encodes an integer containing the max relevance"?

Okay.  That's less backwards than my sentence.  Done.

https://codereview.chromium.org/22354006/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:221: LOG(WARNING) << "Malformed
MaxRelevance string: " << max_relevance_str;
On 2013/08/09 23:33:15, Peter Kasting wrote:
> While here, consider removing this LOG unless you have plans for how you will
> gather the logging data and make use of it.

Good point.  This warning is not useful.  Removed.

Also removed two other similar LOG(WARNING)s elsewhere in this file while I was
at it.

[commit-bot: I haz the power, 2013-08-10 16:55:43]

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

[commit-bot: I haz the power, 2013-08-12 05:33:46]

Change committed as 216938