Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

This is a re-land of
https://codereview.chromium.org/471673002/
which was reverted because it caused a failure in
interactive_ui_tests
InstantExtendedPrefetchTest.SetPrefetchQuery (run #1):
[ RUN      ] InstantExtendedPrefetchTest.SetPrefetchQuery
../../chrome/browser/ui/search/instant_extended_interactive_uitest.cc:867: Failure
Value of: SearchProvider::ShouldPrefetch(*( omnibox()->model()->result().default_match()))
  Actual: false
Expected: true
[  FAILED  ] InstantExtendedPrefetchTest.SetPrefetchQuery, where TypeParam =  and GetParam() =  (1613 ms)

http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests&number=12979

It has two changes:
- revises the failing tests
- does some requested cleanup of the unit tests

The original changelist description follows:
---

Calls to the suggest server may normally result in a new inline
autocompletion.  This can be disruptive because it means pressing enter
may bring the user to different places depending on how long he/she
waits after typing the last key.

This change prevents new suggestions from becoming the default match.
In other words, the default match is only allowed to change on a
keystroke, not due to a reply coming back from the server.

The consequence of this change is that if previously we'd show an
inline suggestion on a server reply, now we only show it one keystroke
later.  I think this trade-off (one keystroke versus inconsistent
omnibox behavior) is a good one to make.

We still end up with default matches (inline autocompletions within
the omnibox) from the suggest server after this change.  Here's an
example of why:

User types "facebo"
We send a suggest server request.
Server asynchronously returns "facebook" as a top suggestion,
  beating the server-provided verbatim score for "facebo".
We decide not to show it within the omnibox.  It's instead shown
  somewhere in the dropdown.
User types "o".
We send a suggest server request.
We reuse our cached suggestions and suggestion scores.  <<< THE KEY
We show "facebook" as an inline suggestion because it beats
  the default verbatim score that gets assigned to "faceboo".
  (This is the score that we assign by default without having
  yet received the most recent suggest server response.)
We receive the response, which includes "facebook" as a top
  suggestion, beating the server-provided verbatim score
  for "faceboo".
We show "facebook" as an inline suggestion.  i.e., we decide
  not to demote it because it was already being shown inline

TESTED:
unit tests plus interactive tests (facebook.com/l, google.com/a)

BUG=398135

Review URL: https://codereview.chromium.org/481693004

Cr-Commit-Position: refs/heads/master@{#293049}

chrome/browser/ui/search/instant_extended_interactive_uitest.cc

@@ -834,17 +834,19 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedPrefetchTest, SetPrefetchQuery) {
   observer.Wait();
 
   // Set the fake response for suggest request. Response has prefetch details.
-  // Ensure that the page received the prefetch query.
+  // Ensure that the page received the suggest response, then add another
+  // keystroke to allow the asynchronously-received inline autocomplete
+  // suggestion to actually be inlined (which in turn triggers it to prefetch).
   fake_factory()->SetFakeResponse(
-      instant_url().Resolve("#q=pupp"),
-      "[\"pupp\",[\"puppy\", \"puppies\"],[],[],"
+      instant_url().Resolve("#q=pup"),
+      "[\"pup\",[\"puppy\", \"puppies\"],[],[],"
       "{\"google:clientdata\":{\"phi\": 0},"
           "\"google:suggesttype\":[\"QUERY\", \"QUERY\"],"
           "\"google:suggestrelevance\":[1400, 9]}]",
       net::HTTP_OK,
       net::URLRequestStatus::SUCCESS);
 
-  SetOmniboxText("pupp");
+  SetOmniboxText("pup");
   while (!omnibox()->model()->autocomplete_controller()->done()) {
     content::WindowedNotificationObserver ready_observer(
         chrome::NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY,
@@ -852,6 +854,7 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedPrefetchTest, SetPrefetchQuery) {
             omnibox()->model()->autocomplete_controller()));
     ready_observer.Wait();
   }
+  SetOmniboxText("pupp");
 
   ASSERT_EQ(3, CountSearchProviderSuggestions());
   content::WebContents* active_tab =

components/omnibox/base_search_provider.cc

@@ -105,14 +105,17 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     bool from_keyword_provider,
     const TemplateURL* template_url,
     const SearchTermsData& search_terms_data) {
-  // This call uses a number of default values.  For instance, it assumes that
-  // if this match is from a keyword provider than the user is in keyword mode.
+  // These calls use a number of default values.  For instance, they assume
+  // that if this match is from a keyword provider, then the user is in keyword
+  // mode.  They also assume the caller knows what it's doing and we set
+  // this match to look as if it was received/created synchronously.
+  SearchSuggestionParser::SuggestResult suggest_result(
+      suggestion, type, suggestion, base::string16(), base::string16(),
+      base::string16(), base::string16(), std::string(), std::string(),
+      from_keyword_provider, 0, false, false, base::string16());
+  suggest_result.set_received_after_last_keystroke(false);
   return CreateSearchSuggestion(
-      NULL, AutocompleteInput(), from_keyword_provider,
-      SearchSuggestionParser::SuggestResult(
-          suggestion, type, suggestion, base::string16(), base::string16(),
-          base::string16(), base::string16(), std::string(), std::string(),
-          from_keyword_provider, 0, false, false, base::string16()),
+      NULL, AutocompleteInput(), from_keyword_provider, suggest_result,
       template_url, search_terms_data, 0, false);
 }
 
@@ -233,7 +236,10 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     match.fill_into_edit.assign(base::ASCIIToUTF16("?"));
   if (suggestion.from_keyword_provider())
     match.fill_into_edit.append(match.keyword + base::char16(' '));
+  // We only allow inlinable navsuggestions that were received before the
+  // last keystroke because we don't want asynchronous inline autocompletions.
   if (!input.prevent_inline_autocomplete() &&
+      !suggestion.received_after_last_keystroke() &&
       (!in_keyword_mode || suggestion.from_keyword_provider()) &&
       StartsWith(suggestion.suggestion(), input.text(), false)) {
     match.inline_autocompletion =

components/omnibox/search_provider.h

@@ -75,10 +75,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);
@@ -139,20 +140,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;
@@ -207,12 +207,16 @@ 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);
+
+  // Given new asynchronous results, ensure that we don't clobber the current
+  // top results, which were determined synchronously on the last keystroke.
+  void PersistTopSuggestions(SearchSuggestionParser::Results* results);
 
   // Apply calculated relevance scores to the current results.
-  void ApplyCalculatedRelevance();
   void ApplyCalculatedSuggestRelevance(
       SearchSuggestionParser::SuggestResults* list);
   void ApplyCalculatedNavigationRelevance(
@@ -351,6 +355,11 @@ class SearchProvider : public BaseSearchProvider,
   SearchSuggestionParser::Results default_results_;
   SearchSuggestionParser::Results keyword_results_;
 
+  // The top query suggestion, left blank if none.
+  base::string16 top_query_suggestion_match_contents_;
+  // The top navigation suggestion, left blank/invalid if none.
+  GURL top_navigation_suggestion_;
+
   GURL current_page_url_;
 
   // Session token management.

components/omnibox/search_suggestion_parser.cc

@@ -50,6 +50,7 @@ SearchSuggestionParser::Result::Result(bool from_keyword_provider,
       type_(type),
       relevance_(relevance),
       relevance_from_server_(relevance_from_server),
+      received_after_last_keystroke_(true),
       deletion_url_(deletion_url) {}
 
 SearchSuggestionParser::Result::~Result() {}

components/omnibox/search_suggestion_parser.h

@@ -55,6 +55,13 @@ class SearchSuggestionParser {
     AutocompleteMatchType::Type type() const { return type_; }
     int relevance() const { return relevance_; }
     void set_relevance(int relevance) { relevance_ = relevance; }
+    bool received_after_last_keystroke() const {
+      return received_after_last_keystroke_;
+    }
+    void set_received_after_last_keystroke(
+        bool received_after_last_keystroke) {
+      received_after_last_keystroke_ = received_after_last_keystroke;
+    }
 
     bool relevance_from_server() const { return relevance_from_server_; }
     void set_relevance_from_server(bool relevance_from_server) {
@@ -91,6 +98,11 @@ class SearchSuggestionParser {
     // there.
     bool relevance_from_server_;
 
+    // Whether this result was received asynchronously after the last
+    // keystroke, otherwise it must have come from prior cached results
+    // or from a synchronous provider.
+    bool received_after_last_keystroke_;
+
     // Optional deletion URL provided with suggestions. Fetching this URL
     // should result in some reasonable deletion behaviour on the server,
     // e.g. deleting this term out of a user's server-side search history.