Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 802b879de05b6d5c94107609cc2a4dc934d5a0d2..4fb62b14d2a6a6698e79626a0b57b9eede489b56 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -170,6 +170,33 @@ int SearchProvider::CalculateRelevanceForKeywordVerbatim(
   return (type == metrics::OmniboxInputType::QUERY) ? 1450 : 1100;
 }
 
+// static
+void SearchProvider::RemoveOrReviseOldResultsForOneProvider(
+    bool minimal_changes, SearchSuggestionParser::Results* results) {
+  // When called without |minimal_changes|, it likely means the user has
+  // pressed a key.  Revise the cached results appropriately.
+  if (!minimal_changes) {
+    for (SearchSuggestionParser::SuggestResults::iterator sug_it =
+             results->suggest_results.begin();
+         sug_it != results->suggest_results.end(); ++sug_it) {
+      sug_it->set_received_after_last_keystroke(false);
+    }
+    for (SearchSuggestionParser::NavigationResults::iterator nav_it =
+             results->navigation_results.begin();
+         nav_it != results->navigation_results.end(); ++nav_it) {
+      nav_it->set_received_after_last_keystroke(false);
+    }
+  }
+}
+
+// static
+ACMatches::iterator SearchProvider::FindTopMatch(ACMatches* matches) {
+  ACMatches::iterator it = matches->begin();
+  while ((it != matches->end()) && !it->allowed_to_be_default_match)
+    ++it;
+  return it;
+}
+
 void SearchProvider::Start(const AutocompleteInput& input,
                            bool minimal_changes) {
   // Do our best to load the model as early as possible.  This will reduce
@@ -392,6 +419,8 @@ void SearchProvider::LogFetchComplete(bool success, bool is_keyword) {
 }
 
 void SearchProvider::UpdateMatches() {
+  PersistGoodResultsForOneProvider(&default_results_);
+  PersistGoodResultsForOneProvider(&keyword_results_);
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
@@ -401,11 +430,12 @@ void SearchProvider::UpdateMatches() {
     // These blocks attempt to repair undesirable behavior by suggested
     // relevances with minimal impact, preserving other suggested relevances.
 
-    if (!HasKeywordDefaultMatchInKeywordMode()) {
+    if ((providers_.GetKeywordProviderURL() != NULL) &&
+        (FindTopMatch() == matches_.end())) {
       // In keyword mode, disregard the keyword verbatim suggested relevance
-      // if necessary so there at least one keyword match that's allowed to
-      // be the default match.
-      keyword_results_.verbatim_relevance = -1;
+      // if necessary, so at least one match is allowed to be default.  Give
+      // it the lowest non-zero score to best reflect what the server desired.
+      keyword_results_.verbatim_relevance = 1;
       ConvertResultsToAutocompleteMatches();
     }
     if (IsTopMatchSearchWithURLInput()) {
@@ -422,28 +452,34 @@ void SearchProvider::UpdateMatches() {
     if (FindTopMatch() == matches_.end()) {
       // Guarantee that SearchProvider returns a legal default match.  (The
       // omnibox always needs at least one legal default match, and it relies
-      // on SearchProvider to always return one.)
-      ApplyCalculatedRelevance();
+      // on SearchProvider to always return one.)  Give them the lowest
+      // non-zero scores to best reflect what the server desired.
+      default_results_.verbatim_relevance = 1;
+      keyword_results_.verbatim_relevance = 1;
       ConvertResultsToAutocompleteMatches();
     }
-    DCHECK(HasKeywordDefaultMatchInKeywordMode());
     DCHECK(!IsTopMatchSearchWithURLInput());
     DCHECK(FindTopMatch() != matches_.end());
   }
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Omnibox.SearchProviderMatches", matches_.size(), 1, 6, 7);
 
-  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();

[Mark P, 2014/08/15 20:04:38]

This was clobbered in the refactoring changelist.

-  if ((keyword_url != NULL) && HasKeywordDefaultMatchInKeywordMode()) {
-    // If there is a keyword match that is allowed to be the default match,
-    // then prohibit default provider matches from being the default match lest
-    // such matches cause the user to break out of keyword mode.
-    for (ACMatches::iterator it = matches_.begin(); it != matches_.end();
-         ++it) {
-      if (it->keyword != keyword_url->keyword())
-        it->allowed_to_be_default_match = false;
+  // Record the inlined suggestion (if any) for future use.
+  inlined_query_suggestion_match_contents_ = base::string16();
+  inlined_navsuggestion_ = GURL();
+  ACMatches::const_iterator first_match = FindTopMatch();
+  if ((first_match != matches_.end()) &&
+      !first_match->inline_autocompletion.empty()) {
+    // Identify if this match came from a query suggestion or a navsuggestion.
+    // In either case, extracts the identifying feature of the suggestion
+    // (query string or navigation url).
+    if (AutocompleteMatch::IsSearchType(first_match->type)) {
+      inlined_query_suggestion_match_contents_ = first_match->contents;
+    } else {
+      inlined_navsuggestion_ = first_match->destination_url;
     }
   }
+
   UpdateDone();
 }
 
@@ -534,8 +570,7 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
   // We can't keep running any previous query, so halt it.
   StopSuggest();
 
-  // Remove existing results that cannot inline autocomplete the new input.
-  RemoveAllStaleResults();
+  RemoveOrReviseOldResults(minimal_changes);
 
   // Update the content classifications of remaining results so they look good
   // against the current input.
@@ -619,21 +654,39 @@ bool SearchProvider::IsQuerySuitableForSuggest() const {
   return true;
 }
 
-void SearchProvider::RemoveAllStaleResults() {
+void SearchProvider::RemoveOrReviseOldResults(bool minimal_changes) {
   if (keyword_input_.text().empty()) {
     // User is either in keyword mode with a blank input or out of
     // keyword mode entirely.
     keyword_results_.Clear();
   }
+  RemoveOrReviseOldResultsForOneProvider(minimal_changes, &default_results_);
+  RemoveOrReviseOldResultsForOneProvider(minimal_changes, &keyword_results_);
 }
 
-void SearchProvider::ApplyCalculatedRelevance() {
-  ApplyCalculatedSuggestRelevance(&keyword_results_.suggest_results);
-  ApplyCalculatedSuggestRelevance(&default_results_.suggest_results);
-  ApplyCalculatedNavigationRelevance(&keyword_results_.navigation_results);
-  ApplyCalculatedNavigationRelevance(&default_results_.navigation_results);
-  default_results_.verbatim_relevance = -1;
-  keyword_results_.verbatim_relevance = -1;
+void SearchProvider::PersistGoodResultsForOneProvider(

[msw, 2014/08/15 21:04:37]

Why do we need this at all? Isn't "RemoveOrReviseOldResultsForOneProvider"
sufficient? If this is needed, consider another function name that better
matches the operation here, like: PromoteAsyncInlineSuggestions or similar.

[Mark P, 2014/08/15 22:09:21]

This is definitely needed.  Without this function the behavior is correct when
typing "a", getting suggest results, adding "b", and then displaying cached
results (which we are allowed to inline).  But when the suggest reply for "b"
returns, we suddenly forget that we were inlining things before.  Hence, we need
this function to allow us to keep inlining what we were before.

I kind of like the current name.  PromoteAsyncInlineSuggestions() isn't right
because we're not promoting anything.  Renamed to a combination of the two:
PersistInlineSuggestions()

[msw, 2014/08/15 23:31:10]

The new name is good, thanks for explaining the need for this to me.

+    SearchSuggestionParser::Results* results) {
+  // Mark the result corresponding to the previous inline autocompletion as
+  // having been received on a previous keystroke (as indeed it must have

[msw, 2014/08/15 21:04:37]

nit: s/on a previous/before the last/

[Mark P, 2014/08/15 22:09:21]

Done.

+  // because we prohibit newly-received results from becoming the inline

[msw, 2014/08/15 21:04:37]

I don't know how this (indeed it must have...) comment explains why the result
must is guaranteed to have been received before the last keystroke... Wouldn't
that actually be contingent on when this function is called? Can you try to
clarify this parenthesized portion, maybe move it to the callsite, or perhaps
remove it altogether?

[Mark P, 2014/08/15 22:09:21]

Removed the comment.  I think the comment by the function declaration is clear
enough, especially in combination with the new function name.

+  // autocompletion).
+  if (!inlined_query_suggestion_match_contents_.empty()) {
+    for (SearchSuggestionParser::SuggestResults::iterator sug_it =
+             results->suggest_results.begin();
+         sug_it != results->suggest_results.end(); ++sug_it) {
+      if (sug_it->match_contents() ==
+          inlined_query_suggestion_match_contents_)
+        sug_it->set_received_after_last_keystroke(false);
+    }
+  }
+  if (inlined_navsuggestion_.is_valid()) {
+    for (SearchSuggestionParser::NavigationResults::iterator nav_it =
+             results->navigation_results.begin();
+         nav_it != results->navigation_results.end(); ++nav_it) {
+      if (nav_it->url() == inlined_navsuggestion_)
+        nav_it->set_received_after_last_keystroke(false);
+    }
+  }
 }
 
 void SearchProvider::ApplyCalculatedSuggestRelevance(
@@ -730,6 +783,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       default_results_.suggest_results.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
+  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
   if (verbatim_relevance > 0) {
     const base::string16& trimmed_verbatim =
         base::CollapseWhitespace(input_.text(), false);
@@ -755,10 +809,9 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
         answer_type, std::string(), std::string(), false, verbatim_relevance,
         relevance_from_server, false, trimmed_verbatim);
     AddMatchToMap(verbatim, std::string(), did_not_accept_default_suggestion,
-                  false, &map);
+                  false, keyword_url != NULL, &map);
   }
   if (!keyword_input_.text().empty()) {
-    const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
     // We only create the verbatim search query match for a keyword
     // if it's not an extension keyword.  Extension keywords are handled
     // in KeywordProvider::Start().  (Extensions are complicated...)
@@ -780,7 +833,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
             true, keyword_verbatim_relevance, keyword_relevance_from_server,
             false, trimmed_verbatim);
         AddMatchToMap(verbatim, std::string(),
-                      did_not_accept_keyword_suggestion, false, &map);
+                      did_not_accept_keyword_suggestion, false, true, &map);
       }
     }
   }
@@ -802,9 +855,10 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   AddNavigationResultsToMatches(default_results_.navigation_results, &matches);
 
   // Now add the most relevant matches to |matches_|.  We take up to kMaxMatches
-  // suggest/navsuggest matches, regardless of origin.  If Instant Extended is
-  // enabled and we have server-provided (and thus hopefully more accurate)
-  // scores for some suggestions, we allow more of those, until we reach
+  // suggest/navsuggest matches, regardless of origin.  We always include in
+  // that set a legal default match if possible.  If Instant Extended is enabled
+  // and we have server-provided (and thus hopefully more accurate) scores for
+  // some suggestions, we allow more of those, until we reach
   // AutocompleteResult::kMaxMatches total matches (that is, enough to fill the
   // whole popup).
   //
@@ -813,6 +867,13 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   // higher-scoring matches under the conditions above.
   std::sort(matches.begin(), matches.end(), &AutocompleteMatch::MoreRelevant);
   matches_.clear();
+  // Guarantee that if there's a legal default match anywhere in the result

[msw, 2014/08/15 21:04:37]

Why is this needed now? I thought the |allowed_to_be_default_match| behavior
made this type of provider-side fixup unnecessary.

[Mark P, 2014/08/15 22:09:21]

This is needed because AutocompleteResult enforces the behavior: find the top
default match and pull it to the top.  However, it can only do that if there is
a default match to be found.  This change makes sure SearchProvider returns one.
 Previously, we trusted SearchProvider to return one by clobbering relevance
scores willy-nilly and then relying on local scoring having the default match
beat the suggestions.  Now we return the top default match regardless of its
relevance score.  We don't need to clobber relevance scores as wildly as before.

[msw, 2014/08/15 23:31:10]

My question is "why do we need to rotate the matches list?". I guess it's
because the constraint enforcement might now leave the "top match"
(highest-scored-inlinable match) scored lowly and ordered lower down the
|matches| list than kMaxMatches, so the step below would otherwise omit the top
result. Then, wouldn't rotating the list potentially use more lowly
ranked/ordered results instead of the prior sorting? If so, how is that not
caught by a unit test? (ie. a test should ensure that we have an inlinable
match, even if it's the lowest-ranked of N>kMaxMatches results). Instead of
rotating, can we just yank the "top match" to the top of the |matches| list?

[Mark P, 2014/08/15 23:50:15]

This call to rotate does a remove from current location and insert at the
beginning.  In other word, it just yanks it (as you request).  Made more
explicit in the comment.

+  // set that it'll get returned by moving the default match to the front
+  // of the list.
+  ACMatches::iterator default_match = FindTopMatch(&matches);
+  if (default_match != matches.end()) {

[msw, 2014/08/15 21:04:37]

nit: remove curly braces.

[Mark P, 2014/08/15 22:09:21]

Done.

+    std::rotate(matches.begin(), default_match, default_match + 1);
+  }
 
   size_t num_suggestions = 0;
   for (ACMatches::const_iterator i(matches.begin());
@@ -848,21 +909,6 @@ ACMatches::const_iterator SearchProvider::FindTopMatch() const {
   return it;
 }
 
-bool SearchProvider::HasKeywordDefaultMatchInKeywordMode() const {

[Mark P, 2014/08/15 20:04:38]

Deleted in refactoring changelist.

-  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
-  // If the user is not in keyword mode, return true to say that this
-  // constraint is not violated.
-  if (keyword_url == NULL)
-    return true;
-  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
-       ++it) {
-    if ((it->keyword == keyword_url->keyword()) &&
-        it->allowed_to_be_default_match)
-      return true;
-  }
-  return false;
-}
-
 bool SearchProvider::IsTopMatchSearchWithURLInput() const {
   ACMatches::const_iterator first_match = FindTopMatch();
   return (input_.type() == metrics::OmniboxInputType::URL) &&
@@ -922,7 +968,8 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
                                          is_keyword);
   for (SearchSuggestionParser::SuggestResults::const_iterator i(
            scored_results.begin()); i != scored_results.end(); ++i) {
-    AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true, map);
+    AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true,
+                  providers_.GetKeywordProviderURL() != NULL, map);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
                       base::TimeTicks::Now() - start_time);
@@ -965,13 +1012,15 @@ SearchSuggestionParser::SuggestResults SearchProvider::ScoreHistoryResults(
       found_what_you_typed_match = true;
       insertion_position = scored_results.begin();
     }
-    scored_results.insert(
-        insertion_position,
-        SearchSuggestionParser::SuggestResult(
-            trimmed_suggestion, AutocompleteMatchType::SEARCH_HISTORY,
-            trimmed_suggestion, base::string16(), base::string16(),
-            base::string16(), base::string16(), std::string(), std::string(),
-            is_keyword, relevance, false, false, trimmed_input));
+    SearchSuggestionParser::SuggestResult history_suggestion(
+        trimmed_suggestion, AutocompleteMatchType::SEARCH_HISTORY,
+        trimmed_suggestion, base::string16(), base::string16(),
+        base::string16(), base::string16(), std::string(), std::string(),
+        is_keyword, relevance, false, false, trimmed_input);
+    // History results are synchronous; they are received at the last keystroke,

[msw, 2014/08/15 21:04:37]

nit: s/upon/with/ remove ", not after it"

[Mark P, 2014/08/15 22:09:21]

Did the latter.

The former fails to apply.   There is no "upon" in the comment.  Left the
comment with "at" because I think it's better than "with".

[msw, 2014/08/15 23:31:10]

I meant to encourage "upon the last keystroke", but what you have is fine.

[Mark P, 2014/08/15 23:50:15]

Switched to "upon".

+    // not after it.
+    history_suggestion.set_received_after_last_keystroke(false);
+    scored_results.insert(insertion_position, history_suggestion);
   }
 
   // History returns results sorted for us.  However, we may have docked some
@@ -1044,8 +1093,10 @@ void SearchProvider::AddSuggestResultsToMap(
     const SearchSuggestionParser::SuggestResults& results,
     const std::string& metadata,
     MatchMap* map) {
-  for (size_t i = 0; i < results.size(); ++i)
-    AddMatchToMap(results[i], metadata, i, false, map);
+  for (size_t i = 0; i < results.size(); ++i) {
+    AddMatchToMap(results[i], metadata, i, false,
+                  providers_.GetKeywordProviderURL() != NULL, map);
+  }
 }
 
 int SearchProvider::GetVerbatimRelevance(bool* relevance_from_server) const {
@@ -1204,14 +1255,18 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
   }
   // An inlineable navsuggestion can only be the default match when there
   // is no keyword provider active, lest it appear first and break the user
-  // out of keyword mode.  It can also only be default if either the inline
+  // out of keyword mode.  We also must have received the navsuggestion before
+  // the last keystroke.  (We don't want asynchronous inline autocompletions.)
+  // The navsuggestion can also only be default if either the inline
   // autocompletion is empty or we're not preventing inline autocompletion.
   // Finally, if we have an inlineable navsuggestion with an inline completion
   // that we're not preventing, make sure we didn't trim any whitespace.
   // We don't want to claim http://foo.com/bar is inlineable against the
   // input "foo.com/b ".
-  match.allowed_to_be_default_match = (prefix != NULL) &&
+  match.allowed_to_be_default_match =
+      (prefix != NULL) &&
       (providers_.GetKeywordProviderURL() == NULL) &&
+      !navigation.received_after_last_keystroke() &&
       (match.inline_autocompletion.empty() ||
       (!input_.prevent_inline_autocomplete() && !trimmed_whitespace));
   match.EnsureUWYTIsAllowedToBeDefault(