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 90172527f321754687cf3324791051f2668f0fa3..8dad268d03b712e4305be6ec30b21bd753588d0c 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -162,6 +162,34 @@ int SearchProvider::CalculateRelevanceForKeywordVerbatim(
   return (type == metrics::OmniboxInputType::QUERY) ? 1450 : 1100;
 }
 
+// static
+void SearchProvider::UpdateOldResults(
+    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
@@ -391,6 +419,8 @@ void SearchProvider::LogFetchComplete(bool success, bool is_keyword) {
 }
 
 void SearchProvider::UpdateMatches() {
+  PersistTopSuggestions(&default_results_);
+  PersistTopSuggestions(&keyword_results_);
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
@@ -408,8 +438,10 @@ void SearchProvider::UpdateMatches() {
       // In non-extension keyword mode, disregard the keyword verbatim suggested
       // relevance if necessary, so at least one match is allowed to be default.
       // (In extension keyword mode this is not necessary because the extension
-      // will return a default match.)
-      keyword_results_.verbatim_relevance = -1;
+      // will return a default match.)  Give keyword verbatim the lowest
+      // non-zero score to best reflect what the server desired.
+      DCHECK_EQ(0, keyword_results_.verbatim_relevance);
+      keyword_results_.verbatim_relevance = 1;
       ConvertResultsToAutocompleteMatches();
     }
     if (IsTopMatchSearchWithURLInput()) {
@@ -428,8 +460,13 @@ void SearchProvider::UpdateMatches() {
       // when in extension-based keyword mode).  The omnibox always needs at
       // least one legal default match, and it relies on SearchProvider in
       // combination with KeywordProvider (for extension-based keywords) to
-      // always return one.
-      ApplyCalculatedRelevance();
+      // always return one.  Give the verbatim suggestion the lowest non-zero
+      // scores to best reflect what the server desired.
+      DCHECK_EQ(0, default_results_.verbatim_relevance);
+      default_results_.verbatim_relevance = 1;
+      // We do not have to alter keyword_results_.verbatim_relevance here.
+      // If the user is in keyword mode, we already reverted (earlier in this
+      // function) the instructions to suppress keyword verbatim.
       ConvertResultsToAutocompleteMatches();
     }
     DCHECK(!IsTopMatchSearchWithURLInput());
@@ -437,6 +474,22 @@ void SearchProvider::UpdateMatches() {
   }
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Omnibox.SearchProviderMatches", matches_.size(), 1, 6, 7);
+
+  // Record the top suggestion (if any) for future use.
+  top_query_suggestion_match_contents_ = base::string16();
+  top_navigation_suggestion_ = 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))
+      top_query_suggestion_match_contents_ = first_match->contents;
+    else
+      top_navigation_suggestion_ = first_match->destination_url;
+  }
+
   UpdateDone();
 }
 
@@ -524,8 +577,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();
+  UpdateAllOldResults(minimal_changes);
 
   // Update the content classifications of remaining results so they look good
   // against the current input.
@@ -609,21 +661,39 @@ bool SearchProvider::IsQuerySuitableForSuggest() const {
   return true;
 }
 
-void SearchProvider::RemoveAllStaleResults() {
+void SearchProvider::UpdateAllOldResults(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();
   }
+  UpdateOldResults(minimal_changes, &default_results_);
+  UpdateOldResults(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::PersistTopSuggestions(
+    SearchSuggestionParser::Results* results) {
+  // Mark any results matching the current top results as having been received
+  // prior to the last keystroke.  That prevents asynchronous updates from
+  // clobbering top results, which may be used for inline autocompletion.
+  // Other results don't need similar changes, because they shouldn't be
+  // displayed asynchronously anyway.
+  if (!top_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() == top_query_suggestion_match_contents_)
+        sug_it->set_received_after_last_keystroke(false);
+    }
+  }
+  if (top_navigation_suggestion_.is_valid()) {
+    for (SearchSuggestionParser::NavigationResults::iterator nav_it =
+             results->navigation_results.begin();
+         nav_it != results->navigation_results.end(); ++nav_it) {
+      if (nav_it->url() == top_navigation_suggestion_)
+        nav_it->set_received_after_last_keystroke(false);
+    }
+  }
 }
 
 void SearchProvider::ApplyCalculatedSuggestRelevance(
@@ -792,9 +862,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).
   //
@@ -803,6 +874,12 @@ 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
+  // set that it'll get returned.  The rotate() call does this by moving the
+  // default match to the front of the list.
+  ACMatches::iterator default_match = FindTopMatch(&matches);
+  if (default_match != matches.end())
+    std::rotate(matches.begin(), default_match, default_match + 1);
 
   size_t num_suggestions = 0;
   for (ACMatches::const_iterator i(matches.begin());
@@ -941,13 +1018,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 upon the last

[msw, 2014/08/28 19:21:40]

nit: s/upon/on/ for a one-liner.

[Mark P, 2014/08/28 20:18:51]

Done.

+    // keystroke.
+    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
@@ -1179,14 +1258,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, to prevent asynchronous inline autocompletions changes.
+  // 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(