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

chrome/browser/autocomplete/history_url_provider.cc

Index: chrome/browser/autocomplete/history_url_provider.cc
diff --git a/chrome/browser/autocomplete/history_url_provider.cc b/chrome/browser/autocomplete/history_url_provider.cc
index 0c395ef89ac1f5746352c488ef4cbbe0bf956bcd..2f7a563834819a42159d7cde11774564651c0d55 100644
--- a/chrome/browser/autocomplete/history_url_provider.cc
+++ b/chrome/browser/autocomplete/history_url_provider.cc
@@ -568,7 +568,7 @@ void HistoryURLProvider::Start(const AutocompleteInput& input,
   if (url_db) {
     DoAutocomplete(NULL, url_db, params.get());
     matches_.clear();
-    PromoteMatchIfNecessary(*params);
+    PromoteMatchesIfNecessary(*params);
     // NOTE: We don't reset |params| here since at least the |promote_type|
     // field on it will be read by the second pass -- see comments in
     // DoAutocomplete().
@@ -766,7 +766,7 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
   // searching has been disabled by policy. In the cases where we've parsed as
   // UNKNOWN, we'll still show an accidental search infobar if need be.
   VisitClassifier classifier(this, params->input, db);
-  bool have_what_you_typed_match =
+  params->have_what_you_typed_match =
       (params->input.type() != metrics::OmniboxInputType::QUERY) &&
       ((params->input.type() != metrics::OmniboxInputType::UNKNOWN) ||
        (classifier.type() == VisitClassifier::UNVISITED_INTRANET) ||
@@ -774,7 +774,7 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
        (AutocompleteInput::NumNonHostComponents(params->input.parts()) > 0) ||
        !params->default_search_provider);
   const bool have_shorter_suggestion_suitable_for_inline_autocomplete =
-      PromoteOrCreateShorterSuggestion(db, have_what_you_typed_match, params);
+      PromoteOrCreateShorterSuggestion(db, params);
 
   // Check whether what the user typed appears in history.
   const bool can_check_history_for_exact_match =
@@ -802,13 +802,13 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
   // this input, so we can promote that as the best match.
   if (params->exact_suggestion_is_in_history) {
     params->promote_type = HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH;
-  } else if (!params->prevent_inline_autocomplete && !params->matches.empty() &&
-      (have_shorter_suggestion_suitable_for_inline_autocomplete ||
-       CanPromoteMatchForInlineAutocomplete(params->matches[0]))) {
+  } else if (!params->matches.empty() &&

[Peter Kasting, 2015/03/24 22:02:43]

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.

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

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.

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

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.

[Mark P, 2015/03/25 19:04:57]

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.
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 understand.  Asking someone else to think through these questions with you is
a good idea.
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.

+             (have_shorter_suggestion_suitable_for_inline_autocomplete ||
+              CanPromoteMatchForInlineAutocomplete(params->matches[0]))) {
     params->promote_type = HistoryURLProviderParams::FRONT_HISTORY_MATCH;

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

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.

[Mark P, 2015/03/25 19:04:57]

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

   } else {
     // Failed to promote any URLs.  Use the What You Typed match, if we have it.
-    params->promote_type = have_what_you_typed_match ?
+    params->promote_type = params->have_what_you_typed_match ?
         HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH :
         HistoryURLProviderParams::NEITHER;
   }
@@ -826,15 +826,24 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
   }
 }
 
-void HistoryURLProvider::PromoteMatchIfNecessary(
+void HistoryURLProvider::PromoteMatchesIfNecessary(
     const HistoryURLProviderParams& params) {
   if (params.promote_type == HistoryURLProviderParams::NEITHER)
     return;
-  matches_.push_back(
-      (params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ?
-      params.what_you_typed_match :
-      HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
-                            CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
+  if (params.promote_type == HistoryURLProviderParams::FRONT_HISTORY_MATCH) {
+    matches_.push_back(
+        HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
+                              CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
+  }
+  // Add the what-you-typed match if we're explicitly told to or if we might
+  // need it (because inline autocompletion is disabled, i.e., no other URL
+  // suggestion is allowed to be the default match) and it's eligible for
+  // display.

[Peter Kasting, 2015/03/24 22:02:43]

After thinking more, this code is correct.

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

Acknowledged.

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

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:57]

Done with trivial edits.

+  if ((params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ||
+      (params.prevent_inline_autocomplete &&
+       params.have_what_you_typed_match)) {
+    matches_.push_back(params.what_you_typed_match);
+  }
 }
 
 void HistoryURLProvider::QueryComplete(
@@ -855,7 +864,7 @@ void HistoryURLProvider::QueryComplete(
   // match in it, whereas |params->matches| will be empty.
   if (!params->failed) {
     matches_.clear();
-    PromoteMatchIfNecessary(*params);
+    PromoteMatchesIfNecessary(*params);
 
     // Determine relevance of highest scoring match, if any.
     int relevance = matches_.empty() ?
@@ -956,7 +965,6 @@ bool HistoryURLProvider::CanFindIntranetURL(
 
 bool HistoryURLProvider::PromoteOrCreateShorterSuggestion(
     history::URLDatabase* db,
-    bool have_what_you_typed_match,
     HistoryURLProviderParams* params) {
   if (params->matches.empty())
     return false;  // No matches, nothing to do.
@@ -966,7 +974,7 @@ bool HistoryURLProvider::PromoteOrCreateShorterSuggestion(
   // the same" as any "what you typed" match.
   const history::HistoryMatch& match = params->matches[0];
   GURL search_base = ConvertToHostOnly(match, params->input.text());
-  bool can_add_search_base_to_matches = !have_what_you_typed_match;
+  bool can_add_search_base_to_matches = !params->have_what_you_typed_match;
   if (search_base.is_empty()) {
     // Search from what the user typed when we couldn't reduce the best match
     // to a host.  Careful: use a substring of |match| here, rather than the