Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

chrome/browser/autocomplete/search_provider.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file contains the Search autocomplete provider.  This provider is
 // responsible for all autocomplete entries that start with "Search <engine>
 // for ...", including searching for the current input string, search
 // history, and search suggestions.  An instance of it gets created and
 // managed by the autocomplete controller.
 
@@ skipping 56 matching lines @@
   void set_current_page_url(const GURL& current_page_url) {
     current_page_url_ = current_page_url;
   }
 
  protected:
   virtual ~SearchProvider();
 
  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);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SessionToken);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, AnswersCache);
   FRIEND_TEST_ALL_PREFIXES(AutocompleteProviderTest, GetDestinationURL);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedPrefetchTest, ClearPrefetchedResults);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedPrefetchTest, SetPrefetchQuery);
 
   // Manages the providers (TemplateURLs) used by SearchProvider. Two providers
@@ skipping 45 matching lines @@
 
   class CompareScoredResults;
 
   struct AnswersQueryData {
     base::string16 full_query_text;
     base::string16 query_type;
   };
 
   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;
 
   // BaseSearchProvider:
   virtual const TemplateURL* GetTemplateURL(bool is_keyword) const OVERRIDE;
   virtual const AutocompleteInput GetInput(bool is_keyword) const OVERRIDE;
   virtual bool ShouldAppendExtraParams(
       const SearchSuggestionParser::SuggestResult& result) const OVERRIDE;
   virtual void StopSuggest() OVERRIDE;
@@ skipping 28 matching lines @@
   // Determines whether an asynchronous subcomponent query should run for the
   // current input.  If so, starts it if necessary; otherwise stops it.
   // NOTE: This function does not update |done_|.  Callers must do so.
   void StartOrStopSuggestQuery(bool minimal_changes);
 
   // Returns true when the current query can be sent to the Suggest service.
   // This will be false e.g. when Suggest is disabled, the query contains
   // 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(
       SearchSuggestionParser::NavigationResults* list);
 
   // Starts a new URLFetcher requesting suggest results from |template_url|;
   // callers own the returned URLFetcher, which is NULL for invalid providers.
   net::URLFetcher* CreateSuggestFetcher(int id,
                                         const TemplateURL* template_url,
                                         const AutocompleteInput& input);
 
   // Converts the parsed results to a set of AutocompleteMatches, |matches_|.
   void ConvertResultsToAutocompleteMatches();
 
   // Returns an iterator to the first match in |matches_| which might
   // 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
   // |navigation_results| to matches and adds them to |matches|.
   void AddNavigationResultsToMatches(
       const SearchSuggestionParser::NavigationResults& navigation_results,
       ACMatches* matches);
 
   // Adds a match for each result in |results| to |map|. |is_keyword| indicates
   // whether the results correspond to the keyword provider or default provider.
@@ skipping 97 matching lines @@
   base::TimeTicks time_suggest_request_sent_;
 
   // Fetchers used to retrieve results for the keyword and default providers.
   scoped_ptr<net::URLFetcher> keyword_fetcher_;
   scoped_ptr<net::URLFetcher> default_fetcher_;
 
   // Results from the default and keyword search providers.
   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.
   std::string current_token_;
   base::TimeTicks token_expiration_time_;
 
   // Answers prefetch management.
   AnswersQueryData prefetch_data_;     // Data to use for query prefetching.
   AnswersQueryData last_answer_seen_;  // Last answer seen.
 
   DISALLOW_COPY_AND_ASSIGN(SearchProvider);
 };
 
 #endif  // CHROME_BROWSER_AUTOCOMPLETE_SEARCH_PROVIDER_H_