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 d7be6bf878ddb824e3321dd1da30c6541f97bce4..e0cd23f2de125f56a35b1e34b88dd062b5d81131 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,21 @@ 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(

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

This function implementation was clobbered a long time ago.  Apparently I forgot
to delete the declaration.  Now I'm doing so.

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

Acknowledged.

-      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 RemoveOrReviseOldResults() that acts only on results

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

nit: remove "that acts only on results from one provider"

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

Done.

+  // from one provider.
+  static void RemoveOrReviseOldResultsForOneProvider(

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

Consider UpdateOldResults, CleanupStaleResults, PromoteAsyncResults or similar.
The function name should better match what's going on.

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

Chose your first suggestion.  Done.

+      bool minimal_changes, SearchSuggestionParser::Results* results);

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

nit: one param per line

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

Now moot.

+
+  // Returns an iterator to the first match in |matches| which might

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

nit: remove "an iterator to " for a one-liner.

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

Done.

+  // be chosen as default.
+  static ACMatches::iterator FindTopMatch(ACMatches* matches);
+
   // AutocompleteProvider:
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes) OVERRIDE;
@@ -205,12 +207,18 @@ 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();

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

RemoveOrReviseOldResults() remained to RemoveOrReviseOldResults()

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

Acknowledged.

+  // 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 RemoveOrReviseOldResults(bool minimal_changes);

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

The name here could probably better match the operation, like the helper.

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

Named it UpdateAllOldResults(), to make it parallel with the helper
UpdateOldResults() yet distinguish it from the helper.

+
+  // 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).
+  void PersistGoodResultsForOneProvider(
+      SearchSuggestionParser::Results* results);
 
   // Apply calculated relevance scores to the current results.
-  void ApplyCalculatedRelevance();

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

No longer called in the .cc file after this change.

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

Acknowledged.

   void ApplyCalculatedSuggestRelevance(
       SearchSuggestionParser::SuggestResults* list);
   void ApplyCalculatedNavigationRelevance(
@@ -229,9 +237,9 @@ class SearchProvider : public BaseSearchProvider,
   // be chosen as default.
   ACMatches::const_iterator FindTopMatch() const;
 
-  // Checks if suggested relevances violate certain expected constraints.
-  // See UpdateMatches() for the use and explanation of these constraints.
-  bool HasKeywordDefaultMatchInKeywordMode() const;
+  // Checks if suggested relevances violate an expected constraint.
+  // See UpdateMatches() for the use and explanation of this constraint
+  // and other constraints enforced without the use of helper functions.
   bool IsTopMatchSearchWithURLInput() const;
 
   // Converts an appropriate number of navigation results in
@@ -349,6 +357,11 @@ class SearchProvider : public BaseSearchProvider,
   SearchSuggestionParser::Results default_results_;
   SearchSuggestionParser::Results keyword_results_;
 
+  // The inlined query suggestion (if any), left blank if none.

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

nit: remove "(if any)" here and below; "left blank if none" should suffice.

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

nit: should this be "inline" instead of "inlined" here and below? (if it means
the suggested inline [query/nav] that may not [yet] actually be inlined).

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

I think "inlined" is better.  This result is actually inlined (or at least
SearchProvider thinks it is, assuming it's not beaten by something from another
provider).  This is no "yet" involved here.

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

Done in both locations.

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

Acknowledged.

+  base::string16 inlined_query_suggestion_match_contents_;
+  // The inlined navsuggestion (if any), left blank/invalid if none.

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

nit: "navigation suggestion"

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

Done.

+  GURL inlined_navsuggestion_;

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

nit: inline_navigation_suggestion_match_contents_ (or similar).

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

Switched to inlined_navigation_suggestion.
Left inlineD per my comment above.
Would prefer to do navigation suggestion comparisons using the URL.  After all,
that's what we think of first when we think of a navsuggestion.  (Likewise, when
we think of a query suggestion, we think of the search string, and that's what's
stored in match_contents and displayed to the user.)

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

Acknowledged.

+
   GURL current_page_url_;
 
   // Session token management.