Omnibox: Add Field Trial to Create UWYT When Inlining

Omnibox: Add Field Trial to Create UWYT When Inlining

Currently when HistoryURL provider decides to promote a match
for inline autocompletion, it will then decline to offer a what-you-typed
match even if it's valid and reasonable.

This behavior can cause problems in DisableInling mode because
this means that sometimes HistoryURL provider will not create a UWYT
match. Consequently, the default match in this mode can become a
search-what-you-typed even when the input is a valid URL. This causes
all sorts of badness and irritation.

This changelist creates a new mechanism, controlled through a field
trial parameter, that forces creation of the UWYT match if it wasn't
added due to this reason (promoted URL for inline autocompletion).

This is one approach to implement this strategy. The other approach
is in https://codereview.chromium.org/321243003/

Did some basic tests using a local field trial server with the
proper params.

TBR=pkasting
(to get this in M-37)


BUG=380357

[Mark P, 2014-06-20 04:42:10]

I think this is the most straightforward way to implement what I want.  Please
take a look ASAP.

This way adds a new parameter to HistoryURLProviderParams.

I wasn't sure if you'd like so I also implemented an alternate approach
that requires slightly more logic elsewhere but does not need
the new parameter.  You can find a link to it in the changelist description.

FWIW, I slightly prefer the new parameter version.  It feels cleaner.

--mark

[Peter Kasting, 2014-06-20 21:12:42]

This route is cleaner, so let's go with it, but please remember to fully undo
all this once the field trial is complete.

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:718: // We consider the what
you typed match to be eligible to be displayed when
Nit: to be eligible to be displayed -> eligible for display

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.h (right):

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.h:162: // only useful when
|promote_type| is FRONT_HISTORY_MATCH.
Nit: I think this comment would be clearer as:

True if |what_you_typed_match| is eligible for display.  If this is true,
PromoteMatchesIfNecessary() may choose to place |what_you_typed_match| on
|matches_| even when |promote_type| is not WHAT_YOU_TYPED_MATCH.

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.h:256: // typed matche
immediately after.
Nit: How about:

May promote the what you typed match, the first history match, or both to the
front of |matches_|, depending on the values of params->promote_type and
params->have_what_you_typed_match.

(I don't think we need to mention field trial state here as this isn't meant to
be an exhaustive guide, but if you want to, you could add ", as well as field
trial state." to the end.)

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.h:308: // false if the experiment
isn't active.
This all sort of confuses me.  It makes it sound like users may have to pick
some random selection number to get the WYT match.  But I thought the idea was,
when disable-inlining is on, we always want the WYT match to be default if the
user's input is a URL, so we'll provide the WYT and have the reorder code swap
it up as necessary.  If so, the comment doesn't really sound like that.

[Mark P, 2014-06-23 22:23:12]

Here are the comments.  I will send the change with them for your review on a
separate changelist.  (This one has already been submitted.)

--mark

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:718: // We consider the what
you typed match to be eligible to be displayed when
On 2014/06/20 21:12:42, Peter Kasting wrote:
> Nit: to be eligible to be displayed -> eligible for display

Done.

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.h (right):

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.h:162: // only useful when
|promote_type| is FRONT_HISTORY_MATCH.
On 2014/06/20 21:12:42, Peter Kasting wrote:
> Nit: I think this comment would be clearer as:
> 
> True if |what_you_typed_match| is eligible for display.  If this is true,
> PromoteMatchesIfNecessary() may choose to place |what_you_typed_match| on
> |matches_| even when |promote_type| is not WHAT_YOU_TYPED_MATCH.

Yes, that is clearer.  Done.

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.h:256: // typed matche
immediately after.
On 2014/06/20 21:12:42, Peter Kasting wrote:
> Nit: How about:
> 
> May promote the what you typed match, the first history match, or both to the
> front of |matches_|, depending on the values of params->promote_type and
> params->have_what_you_typed_match.
> 
> (I don't think we need to mention field trial state here as this isn't meant
to
> be an exhaustive guide, but if you want to, you could add ", as well as field
> trial state." to the end.)

Okay, omitting the mention of the field trial seems reasonable.  Took your
comment with a minor edit (to mention params->matches).

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/347963002/diff/40001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.h:308: // false if the experiment
isn't active.
On 2014/06/20 21:12:42, Peter Kasting wrote:
> This all sort of confuses me.  It makes it sound like users may have to pick
> some random selection number to get the WYT match.  But I thought the idea
was,
> when disable-inlining is on, we always want the WYT match to be default if the
> user's input is a URL, so we'll provide the WYT and have the reorder code swap
> it up as necessary.  If so, the comment doesn't really sound like that.

This comment attempts to explain what happens when the disable inlining field
trial is active without this field trial being active.  It is a mess.  I've
removed that whole section and replaced it with a clearer single sentence.

[Mark P, 2014-07-11 19:59:54]

This was submitted as
http://src.chromium.org/viewvc/chrome?view=revision&revision=278811

I'm not sure why codereview.chromium.org doesn't show this.

Closing this review.

--mark