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 64eec0e26d4319af46c7e73f0747367ae0ef5e80..da79756ad02446fa4865b2a3c297931bc87f39fe 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -170,6 +170,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
@@ -392,6 +420,8 @@ void SearchProvider::LogFetchComplete(bool success, bool is_keyword) {
 }
 
 void SearchProvider::UpdateMatches() {
+  PersistInlineSuggestions(&default_results_);
+  PersistInlineSuggestions(&keyword_results_);
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
@@ -404,8 +434,12 @@ void SearchProvider::UpdateMatches() {
     if ((providers_.GetKeywordProviderURL() != NULL) &&
         (FindTopMatch() == matches_.end())) {
       // In keyword mode, disregard the keyword verbatim suggested relevance
-      // if necessary, so at least one match is allowed to be default.
-      keyword_results_.verbatim_relevance = -1;
+      // if necessary, so at least one match is allowed to be default.  (This
+      // is only necessary if we were told to suppress keyword verbatim and
+      // hence have no default keyword 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()) {
@@ -422,8 +456,13 @@ 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 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());
@@ -431,6 +470,23 @@ void SearchProvider::UpdateMatches() {
   }
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Omnibox.SearchProviderMatches", matches_.size(), 1, 6, 7);
+
+  // Record the inlined suggestion (if any) for future use.

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

nit: s/inlined/top/

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

Done.

+  inlined_query_suggestion_match_contents_ = base::string16();
+  inlined_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)) {

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

optional nit: remove curly braces.

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

Done.

+      inlined_query_suggestion_match_contents_ = first_match->contents;
+    } else {
+      inlined_navigation_suggestion_ = first_match->destination_url;
+    }
+  }
+
   UpdateDone();
 }
 
@@ -521,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.
@@ -606,21 +661,35 @@ 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::PersistInlineSuggestions(
+    SearchSuggestionParser::Results* results) {
+  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_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() == inlined_navigation_suggestion_)
+        nav_it->set_received_after_last_keystroke(false);
+    }
+  }
 }
 
 void SearchProvider::ApplyCalculatedSuggestRelevance(
@@ -789,9 +858,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).
   //
@@ -800,6 +870,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 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());
@@ -938,13 +1014,14 @@ 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.
+    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 +1256,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.)

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

nit: "keystroke, to prevent asynchronous inline autocompletion changes."

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

Done.

+  // 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(