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(
-      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;
 
   // 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();
+  // 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(
       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);
@@ skipping 118 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, 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.
   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_