Omnibox: Make HUP Scoring More Sane in Prevent-Inline Mode

Omnibox: Make HUP Scoring Saner in Prevent-Inline Mode

In particular, make HistoryURL provider scoring
basically the same within regular mode and
prevent-inline-autocomplete mode (under the constraint
that we always need a valid default match first).
For what it's worth, the current code demotes (from
~1410) all non-URL-what-you-typed URLs (non-UWYT)
to 900 in prevent-inline-autocomplete mode.  If
there is a UWYT, though, these results will be
clustered / given a free ride to appear right
below it, scoring ~1200.  After this change, all
non-UWYT results will score the same in
prevent-inline-autocomplete and normal modes.
We instead rely on the allowed_to_be_default_match
bit to reorder results as necessary.

To implement this feature, we need knowledge of
whether the what-you-typed match is reasonable.
This change re-adds a variable
have_what_you_typed_match to HistoryURLParams.  This
variable was previously added in
https://codereview.chromium.org/347963002 for
a field trial and then later removed in
https://codereview.chromium.org/879053002

With this variable, the change is straightforward.
Don't change the scoring in prevent-inline-autocomplete
mode.  Instead, simply make sure the what-you-type
match gets added to the list in prevent-inline-mode.
We add it early on the list (using the Promote()
mechanism) so that it'll always appear in the final
set of suggestions.

Tested using about:omnibox

BUG=421772
Committed: https://crrev.com/b457e3280b81672f2d8233f9e801746ad9a82035
Cr-Commit-Position: refs/heads/master@{#322528}

[Mark P, 2015-03-20 22:59:59]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mark P, 2015-03-20 23:00:45]

Peter, please take a look, thanks.

[Peter Kasting, 2015-03-20 23:30:19]

This should probably get a unittest.

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:778: db,
params->have_what_you_typed_match, params);
Just have this function read |have_what_you_typed_match| from |params|.

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:839: // Add the
what-you-typed match if you we're explicitly told to or if we need
Nit: "you"?

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:845:
params.have_what_you_typed_match)) {
Won't this always add the WYT match for |prevent_inline_autocomplete|, even if
we didn't add anything else?

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.h (right):

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.h:255: // parmas->matches, or
both to the front of |matches_|, depending on the
params

[Mark P, 2015-03-24 18:23:27]

> This should probably get a unittest.

I'm not sure.  We generally don't have tests in HUP that test
scoring code, and this is fundamentally a scoring change.
Whether we add the UWYT match at the right times (prevent
mode or not) is already tested.

If you care, I'll write a test that the score work out right
(and the match is added at the right times) in some simple
examples, but I don't think it'll catch anything not already
covered.

--mark

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:778: db,
params->have_what_you_typed_match, params);
On 2015/03/20 23:30:19, Peter Kasting wrote:
> Just have this function read |have_what_you_typed_match| from |params|.

Silly me for overlooking this.  Done.

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:839: // Add the
what-you-typed match if you we're explicitly told to or if we need
On 2015/03/20 23:30:19, Peter Kasting wrote:
> Nit: "you"?

Fixed.

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:845:
params.have_what_you_typed_match)) {
On 2015/03/20 23:30:19, Peter Kasting wrote:
> Won't this always add the WYT match for |prevent_inline_autocomplete|, even if
> we didn't add anything else?

Yes, it'll always add a WYT match for |prevent_inline_autocomplete| if we have
one we're willing to display (|have_what_you_typed_match|).  Revised comment.

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.h (right):

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.h:255: // parmas->matches, or
both to the front of |matches_|, depending on the
On 2015/03/20 23:30:19, Peter Kasting wrote:
> params

Done.

[Peter Kasting, 2015-03-24 21:05:40]

Fair enough on the tests.

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:845:
params.have_what_you_typed_match)) {
On 2015/03/24 18:23:26, Mark P wrote:
> On 2015/03/20 23:30:19, Peter Kasting wrote:
> > Won't this always add the WYT match for |prevent_inline_autocomplete|, even
if
> > we didn't add anything else?
> 
> Yes, it'll always add a WYT match for |prevent_inline_autocomplete| if we have
> one we're willing to display (|have_what_you_typed_match|).  Revised comment.

Huh.  I'm wondering if that's a potential quality loss.  It seems like in cases
where we didn't previously add this (because it's a poor match), and we haven't
added an inline-autocomplete match we need to outscore, we still shouldn't be
adding this.

Or am I missing a case this will break?

[Mark P, 2015-03-24 21:40:50]

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/50001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:845:
params.have_what_you_typed_match)) {
On 2015/03/24 21:05:39, Peter Kasting wrote:
> On 2015/03/24 18:23:26, Mark P wrote:
> > On 2015/03/20 23:30:19, Peter Kasting wrote:
> > > Won't this always add the WYT match for |prevent_inline_autocomplete|,
even
> if
> > > we didn't add anything else?
> > 
> > Yes, it'll always add a WYT match for |prevent_inline_autocomplete| if we
have
> > one we're willing to display (|have_what_you_typed_match|).  Revised
comment.
> 
> Huh.  I'm wondering if that's a potential quality loss.  It seems like in
cases
> where we didn't previously add this (because it's a poor match), and we
haven't
> added an inline-autocomplete match we need to outscore, we still shouldn't be
> adding this.
> 
> Or am I missing a case this will break?

Are you suggesting adding an && !matches.empty() to the second half of the test?

I don't think that'll change anything.

If we don't have a result that we promote to inline autocompletion, we always
add the WYT match if it's eligible (|params.have_what_you_typed_match|).  See
lines 806-815 in this file.

In other words, if all we have are low-quality matches, we always add the WYT
match if it's eligible.  Please tell me if I'm reading the code wrong.  This new
patch will keep this behavior.

[Peter Kasting, 2015-03-24 22:02:44]

I think I'm going to want to add/change comments here, but what I do depends on
the answer to the below.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:805: } else if
(!params->matches.empty() &&
Rather than just removing the |prevent_inline_autocomplete| check here, I wonder
if we should change it to:

  } else if ((!params->prevent_inline_autocomplete ||
              params->have_what_you_typed_match) && ...

I'm worrying about whether we could promote an inline-autocompleted match when
inline autocomplete is disabled, and we don't have a what-you-typed match to
replace it as potential default.

Or is this unnecessary because in all the cases where this would happen, the
inline-autocompleted match is marked as "can't be default" and the search
provider is guaranteed to return a "can be default" SWYT match?  If that's true,
then we should keep the code as you wrote it and not as I'm suggesting, or we'll
fail to reap the full benefit of your scoring change because we still won't
promote these good history matches as high in these cases.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:841: // display.
After thinking more, this code is correct.

[Mark P, 2015-03-24 22:35:46]

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:805: } else if
(!params->matches.empty() &&
On 2015/03/24 22:02:43, Peter Kasting wrote:
> Rather than just removing the |prevent_inline_autocomplete| check here, I
wonder
> if we should change it to:
> 
>   } else if ((!params->prevent_inline_autocomplete ||
>               params->have_what_you_typed_match) && ...
> 
> I'm worrying about whether we could promote an inline-autocompleted match when
> inline autocomplete is disabled, and we don't have a what-you-typed match to
> replace it as potential default.

This is not a problem.  Promoting has to do with scores.  It doesn't matter
whether we promoted a match or not.  The promoted inline autocomplete match will
not be allowed to be the default match, hence the reorder code in
AutocompleteController will reorder results to put a legal default match first. 
SearchProvider will provide such a match if no other provider does.

I answered this first paragraph question of yours without reading the second.  I
believe in the second paragraph you hypothesized the same answer I gave.

> 
> Or is this unnecessary because in all the cases where this would happen, the
> inline-autocompleted match is marked as "can't be default" and the search
> provider is guaranteed to return a "can be default" SWYT match?  If that's
true,
> then we should keep the code as you wrote it and not as I'm suggesting, or
we'll
> fail to reap the full benefit of your scoring change because we still won't
> promote these good history matches as high in these cases.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:841: // display.
On 2015/03/24 22:02:43, Peter Kasting wrote:
> After thinking more, this code is correct.

Acknowledged.

[Peter Kasting, 2015-03-24 23:05:19]

LGTM

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:805: } else if
(!params->matches.empty() &&
On 2015/03/24 22:35:46, Mark P wrote:
> On 2015/03/24 22:02:43, Peter Kasting wrote:
> > Rather than just removing the |prevent_inline_autocomplete| check here, I
> wonder
> > if we should change it to:
> > 
> >   } else if ((!params->prevent_inline_autocomplete ||
> >               params->have_what_you_typed_match) && ...
> > 
> > I'm worrying about whether we could promote an inline-autocompleted match
when
> > inline autocomplete is disabled, and we don't have a what-you-typed match to
> > replace it as potential default.
> 
> This is not a problem.  Promoting has to do with scores.  It doesn't matter
> whether we promoted a match or not.  The promoted inline autocomplete match
will
> not be allowed to be the default match, hence the reorder code in
> AutocompleteController will reorder results to put a legal default match
first. 
> SearchProvider will provide such a match if no other provider does.

That is fine as long as SearchProvider always actually _does_ provide such a
match.

My worry was that it may not be providing a match in every case where
!have_what_you_typed_match.  I'm more concerned about this than I used to be
because of things like variable verbatim -- I want to make sure that even a
downranked verbatim search score, which scores below more then kMaxMatches other
scores, will still be put back at the top of the list.

I think this is probably OK (or we would probably have issues today in any cases
where (prevent_inline_autocomplete && !have_what_you_typed_match)) but I'm
trying to make sure I've thought through every angle.

There is one weird edge case I can think of.  If we have no default search
provider, and the user types a QUERY, then have_what_you_typed_match will be
false, and any promotion here will result in Bad Things -- really, returning any
results at all will result in Bad Things, since none of them will be allowed to
be default, and we'll hit the DCHECK in the controller that there be at least
one defaultable match among the returned matches.  I think that's already a
problem today, though, since if we could promote this match, then the behavior
today would be to return it as a non-promoted match, so we'd still hit that
DCHECK.  And hopefully we can't have a match here to promote, since hopefully
QUERY means "this can't be fixed up to anything that we can produce a prefix
match for" -- but I'm not sure that's the case, now that we do things like
marking "calculator-like" inputs as QUERY in AutocompleteInput::Parse().

So I guess we don't need to try to address that since I think this patch isn't
making the issue any worse.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:808: params->promote_type =
HistoryURLProviderParams::FRONT_HISTORY_MATCH;
OK, so let's put something like this comment here:

    // Note that we promote this inline-autocompleted match even when
    // params->prevent_inline_autocomplete is true.  This is safe because in
    // this case the match will be marked as "not allowed to be default", and
    // a non-inlined match that is "allowed to be default" will be reordered
    // above it by the controller.  We ensure there is such a match in two ways:
    //   * If params->have_what_you_typed_match is true, we force the
    //     what-you-typed match to be added in this case.  See comments in
    //     PromoteMatchesIfNecessary().
    //   * Otherwise, we should have some sort of QUERY or UNKNOWN input that
    //     the SearchProvider will provide a defaultable WYT match for.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:841: // display.
Let's make the comment here something like this:

  // There are two cases where we need to add the what-you-typed-match:
  //   * If params.promote_type is WHAT_YOU_TYPED_MATCH, we're being explicitly
  //     directed to.
  //   * If params.have_what_you_typed_match is true, then params.promote_type
  //     can't be NEITHER (see code near the end of DoAutocomplete()), so if
  //     it's not WHAT_YOU_TYPED_MATCH, it must be FRONT_HISTORY_MATCH, and
  //     we'll have promoted the history match above.  If
  //     params.prevent_inline_autocomplete is also true, then this match
  //     will be marked "not allowed to be default", and we need to add the
  //     what-you-typed match to ensure there's a defaultable match for the
  //     controller to promote.  (If params.have_what_you_typed_match is false,
  //     the SearchProvider should take care of adding this defaultable match.)

[Mark P, 2015-03-25 19:04:58]

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:805: } else if
(!params->matches.empty() &&
On 2015/03/24 23:05:19, Peter Kasting wrote:
> On 2015/03/24 22:35:46, Mark P wrote:
> > On 2015/03/24 22:02:43, Peter Kasting wrote:
> > > Rather than just removing the |prevent_inline_autocomplete| check here, I
> > wonder
> > > if we should change it to:
> > > 
> > >   } else if ((!params->prevent_inline_autocomplete ||
> > >               params->have_what_you_typed_match) && ...
> > > 
> > > I'm worrying about whether we could promote an inline-autocompleted match
> when
> > > inline autocomplete is disabled, and we don't have a what-you-typed match
to
> > > replace it as potential default.
> > 
> > This is not a problem.  Promoting has to do with scores.  It doesn't matter
> > whether we promoted a match or not.  The promoted inline autocomplete match
> will
> > not be allowed to be the default match, hence the reorder code in
> > AutocompleteController will reorder results to put a legal default match
> first. 
> > SearchProvider will provide such a match if no other provider does.
> 
> That is fine as long as SearchProvider always actually _does_ provide such a
> match.

SearchProvider is always supposed to provide a legal default match (regardless
of input type).  If it doesn't, it's a bug.  We found one such bug recently
(default search engine got renamed/deleted in the middle of processing the
request).  But I think these bugs are rare or non-existent anymore, and
orthogonal to this change.  This change does not change when/how often we put
the UWYT match in HistoryURLProvider's result set.

> My worry was that it may not be providing a match in every case where
> !have_what_you_typed_match.  I'm more concerned about this than I used to be
> because of things like variable verbatim -- I want to make sure that even a
> downranked verbatim search score, which scores below more then kMaxMatches
other
> scores, will still be put back at the top of the list.
It will.  We always pull up a legal default match, even if it wouldn't normally
be in the top X items in the dropdown, and only later resize the list.
https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...

> I think this is probably OK (or we would probably have issues today in any
cases
> where (prevent_inline_autocomplete && !have_what_you_typed_match)) but I'm
> trying to make sure I've thought through every angle.

I understand.  Asking someone else to think through these questions with you is
a good idea.

> There is one weird edge case I can think of.  If we have no default search
> provider, and the user types a QUERY, then have_what_you_typed_match will be
> false, and any promotion here will result in Bad Things -- really, returning
any
> results at all will result in Bad Things, since none of them will be allowed
to
> be default, and we'll hit the DCHECK in the controller that there be at least
> one defaultable match among the returned matches.  I think that's already a
> problem today, though, since if we could promote this match, then the behavior
> today would be to return it as a non-promoted match, so we'd still hit that
> DCHECK.  And hopefully we can't have a match here to promote, since hopefully
> QUERY means "this can't be fixed up to anything that we can produce a prefix
> match for" -- but I'm not sure that's the case, now that we do things like
> marking "calculator-like" inputs as QUERY in AutocompleteInput::Parse().
> 
> So I guess we don't need to try to address that since I think this patch isn't
> making the issue any worse.

I agree.  If the user doesn't have a default search provider, lots of bad things
can happen, and would happen, already today.  This change doesn't make that
situation any worse.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:808: params->promote_type =
HistoryURLProviderParams::FRONT_HISTORY_MATCH;
On 2015/03/24 23:05:19, Peter Kasting wrote:
> OK, so let's put something like this comment here:
> 
>     // Note that we promote this inline-autocompleted match even when
>     // params->prevent_inline_autocomplete is true.  This is safe because in
>     // this case the match will be marked as "not allowed to be default", and
>     // a non-inlined match that is "allowed to be default" will be reordered
>     // above it by the controller.  We ensure there is such a match in two
ways:
>     //   * If params->have_what_you_typed_match is true, we force the
>     //     what-you-typed match to be added in this case.  See comments in
>     //     PromoteMatchesIfNecessary().
>     //   * Otherwise, we should have some sort of QUERY or UNKNOWN input that
>     //     the SearchProvider will provide a defaultable WYT match for.

Okay, that's a nice clarification to add.  Added with only trivial edits.

https://codereview.chromium.org/1022643002/diff/70001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:841: // display.
On 2015/03/24 23:05:19, Peter Kasting wrote:
> Let's make the comment here something like this:
> 
>   // There are two cases where we need to add the what-you-typed-match:
>   //   * If params.promote_type is WHAT_YOU_TYPED_MATCH, we're being
explicitly
>   //     directed to.
>   //   * If params.have_what_you_typed_match is true, then params.promote_type
>   //     can't be NEITHER (see code near the end of DoAutocomplete()), so if
>   //     it's not WHAT_YOU_TYPED_MATCH, it must be FRONT_HISTORY_MATCH, and
>   //     we'll have promoted the history match above.  If
>   //     params.prevent_inline_autocomplete is also true, then this match
>   //     will be marked "not allowed to be default", and we need to add the
>   //     what-you-typed match to ensure there's a defaultable match for the
>   //     controller to promote.  (If params.have_what_you_typed_match is
false,
>   //     the SearchProvider should take care of adding this defaultable
match.)

Done with trivial edits.

[Mark P, 2015-03-25 19:05:04]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2015-03-25 19:05:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1022643002/#ps90001
(title: "Peter's revised comments")

[commit-bot: I haz the power, 2015-03-25 19:05:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022643002/90001

[commit-bot: I haz the power, 2015-03-25 19:45:33]

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

[commit-bot: I haz the power, 2015-03-25 19:45:34]

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

[Mark P, 2015-03-25 23:17:56]

Peter,

I had to revise a few tests.  In the process, I elaborated on them as well.  You
might as well take a look.

thanks,
mark

[Peter Kasting, 2015-03-27 00:48:58]

LGTM (sorry, missed that you'd asked for re-review)

[Mark P, 2015-03-27 03:20:04]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2015-03-27 03:20:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022643002/110001

[commit-bot: I haz the power, 2015-03-27 04:32:17]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-03-27 04:33:09]

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