Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

chrome/browser/autocomplete/search_provider.h

Index: chrome/browser/autocomplete/search_provider.h
diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h
index 7140c5c6ad52f8aef1ad25ea79a1ecc3c8aabb76..a6d8021e6d15feb03d194fdcef0fc641746d2bff 100644
--- a/chrome/browser/autocomplete/search_provider.h
+++ b/chrome/browser/autocomplete/search_provider.h
@@ -74,10 +74,11 @@ class SearchProvider : public BaseSearchProvider,
  private:
   friend class SearchProviderTest;
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, CanSendURL);
+  FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
+                           DontInlineAutocompleteAsynchronously);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, NavigationInline);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, NavigationInlineDomainClassify);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, NavigationInlineSchemeSubstring);
-  FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, RemoveStaleResultsTest);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SuggestRelevanceExperiment);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, TestDeleteMatch);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SuggestQueryUsesToken);
@@ -143,20 +144,19 @@ class SearchProvider : public BaseSearchProvider,
 
   typedef std::vector<history::KeywordSearchTermVisit> HistoryResults;
 
-  // Removes non-inlineable results until either the top result can inline
-  // autocomplete the current input or verbatim outscores the top result.
-  static void RemoveStaleResults(
-      const base::string16& input,
-      int verbatim_relevance,
-      SearchSuggestionParser::SuggestResults* suggest_results,
-      SearchSuggestionParser::NavigationResults* navigation_results);
-
   // Calculates the relevance score for the keyword verbatim result (if the
   // input matches one of the profile's keyword).
   static int CalculateRelevanceForKeywordVerbatim(
       metrics::OmniboxInputType::Type type,
       bool prefer_keyword);
 
+  // A helper function for UpdateAllOldResults().
+  static void UpdateOldResults(bool minimal_changes,
+                               SearchSuggestionParser::Results* results);
+
+  // Returns the first match in |matches| which might be chosen as default.
+  static ACMatches::iterator FindTopMatch(ACMatches* matches);
+
   // AutocompleteProvider:
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes) OVERRIDE;
@@ -205,12 +205,24 @@ class SearchProvider : public BaseSearchProvider,
   // potentially private data, etc.
   bool IsQuerySuitableForSuggest() const;
 
-  // Removes stale results for both default and keyword providers.  See comments
-  // on RemoveStaleResults().
-  void RemoveAllStaleResults();
+  // Remove existing keyword results if the user is no longer in keyword mode,
+  // and, if |minimal_changes| is false, revise the existing results to
+  // indicate they were received before the last keystroke.
+  void UpdateAllOldResults(bool minimal_changes);
+
+  // Loops through all results and marks those that were the previous inline
+  // autocompletion that they were received on a previous keystroke (and
+  // hence can remain as the inline autocompletion).  All other results

[Mark P, 2014/08/15 23:01:06]

I just expanded this comment.  This seems like the best place for a comment line
this.

I might rename PersistInlineSuggestions() simply to PersistSuggestions().  What
do you think?

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

I'd prefer "PersistTopSuggestions", matching my member name suggestions below.

Suggested comment (use this or let it inspire a rewrite):
"Given new asynchronous results, ensure that we don't clobber the current top
results, which were determined synchronously on the last keystroke."

Then, in the function body (or maybe here, I guess):
"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 promoted asynchronously anyway."

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

Done.  Nice comments.

+  // are left marked |received_after_last_keystroke| even if that isn't
+  // strictly true.  (These other results may have been received before the
+  // last keystroke.  When we processed the last keystroke synchronously,
+  // we decided not to inline them because we had better-scoring results.
+  // Upon receiving a new server reply, we leave the old non-inlined results
+  // marked (incorrectly) as |received_after_last_keystroke| because inlining
+  // them asychronously would be surprising.)
+  void PersistInlineSuggestions(SearchSuggestionParser::Results* results);
 
   // Apply calculated relevance scores to the current results.
-  void ApplyCalculatedRelevance();
   void ApplyCalculatedSuggestRelevance(
       SearchSuggestionParser::SuggestResults* list);
   void ApplyCalculatedNavigationRelevance(
@@ -349,6 +361,11 @@ class SearchProvider : public BaseSearchProvider,
   SearchSuggestionParser::Results default_results_;
   SearchSuggestionParser::Results keyword_results_;
 
+  // The inlined query suggestion, left blank if none.

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

nit: consider s/inlined/top/ for comments and identifiers.

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

Done.

+  base::string16 inlined_query_suggestion_match_contents_;
+  // The inlined navigation suggestion, left blank/invalid if none.
+  GURL inlined_navigation_suggestion_;
+
   GURL current_page_url_;
 
   // Session token management.