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..730ddae58d552fa14071ff8d4971161d37a76145 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,8 @@ 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->have_what_you_typed_match, params);

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

Just have this function read |have_what_you_typed_match| from |params|.

[Mark P, 2015/03/24 18:23:26]

Silly me for overlooking this.  Done.

 
   // Check whether what the user typed appears in history.
   const bool can_check_history_for_exact_match =
@@ -802,13 +803,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() &&
+             (have_shorter_suggestion_suitable_for_inline_autocomplete ||
+              CanPromoteMatchForInlineAutocomplete(params->matches[0]))) {
     params->promote_type = HistoryURLProviderParams::FRONT_HISTORY_MATCH;
   } 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 +827,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 you we're explicitly told to or if we need

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

Nit: "you"?

[Mark P, 2015/03/24 18:23:26]

Fixed.

+  // it because we added an inline-autocomplete match yet inline autocompletion
+  // is disabled (so it will not be allowed to be the default match).  Hence,
+  // we need to add the what-you-typed match as a legal default match.
+  if ((params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ||
+      (params.prevent_inline_autocomplete &&
+       params.have_what_you_typed_match)) {

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

Won't this always add the WYT match for |prevent_inline_autocomplete|, even if
we didn't add anything else?

[Mark P, 2015/03/24 18:23:26]

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.

[Peter Kasting, 2015/03/24 21:05:39]

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]

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.

+    matches_.push_back(params.what_you_typed_match);
+  }
 }
 
 void HistoryURLProvider::QueryComplete(
@@ -855,7 +865,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() ?