[omnibox] Implementation of preserve default match during async pass

This CL contains the implementation a for the flag to preserve the
default match during the async pass.

It's a minimally-interventionist approach (the smallest possible
hammer) that simply promotes the previous default match to the top, if
it can be found in the new result set from the async pass.

It won't inject in the old default match if it's truly no longer
present after the async pass, because I think there will be unwanted
side effects from that.

It also adds a test.

Bug: 398135
Change-Id: I9be0f18820436d3d29b8ba7a80835d456d771cc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1788313Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695324}

components/omnibox/browser/autocomplete_controller.cc

@@ -19,6 +19,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/optional.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -548,17 +549,14 @@ void AutocompleteController::UpdateResult(
     bool regenerate_result,
     bool force_notify_default_match_changed) {
   TRACE_EVENT0("omnibox", "AutocompleteController::UpdateResult");
-  const bool last_default_was_valid = result_.default_match() != result_.end();
-  // The following three variables are only set and used if
-  // |last_default_was_valid|.
-  base::string16 last_default_fill_into_edit, last_default_keyword,
-      last_default_associated_keyword;
-  if (last_default_was_valid) {
-    last_default_fill_into_edit = result_.default_match()->fill_into_edit;
-    last_default_keyword = result_.default_match()->keyword;
-    if (result_.default_match()->associated_keyword) {
+
+  base::Optional<AutocompleteMatch> last_default_match;
+  base::string16 last_default_associated_keyword;
+  if (result_.default_match() != result_.end()) {
+    last_default_match = *result_.default_match();
+    if (last_default_match->associated_keyword) {
       last_default_associated_keyword =
-          result_.default_match()->associated_keyword->keyword;
+          last_default_match->associated_keyword->keyword;
     }
   }
 
@@ -579,7 +577,13 @@ void AutocompleteController::UpdateResult(
     result_.ConvertOpenTabMatches(provider_client_.get(), &input_);
 
   // Sort the matches and trim to a small number of "best" matches.
-  result_.SortAndCull(input_, template_url_service_);
+  const AutocompleteMatch* preserve_default_match = nullptr;
+  if (!in_start_ && !regenerate_result && last_default_match &&
+      base::FeatureList::IsEnabled(
+          omnibox::kOmniboxPreserveDefaultMatchAgainstAsyncUpdate)) {
+    preserve_default_match = &last_default_match.value();
+  }
+  result_.SortAndCull(input_, template_url_service_, preserve_default_match);
 
   // Need to validate before invoking CopyOldMatches as the old matches are not
   // valid against the current input.
@@ -616,12 +620,12 @@ void AutocompleteController::UpdateResult(
   // even though the fill into edit hasn't changed (see SearchProvider
   // for one case of this).
   const bool notify_default_match =
-      (last_default_was_valid != default_is_valid) ||
-      (last_default_was_valid &&
+      (last_default_match.has_value() != default_is_valid) ||
+      (last_default_match &&
        ((result_.default_match()->fill_into_edit !=
-          last_default_fill_into_edit) ||
+         last_default_match->fill_into_edit) ||
         (default_associated_keyword != last_default_associated_keyword) ||
-        (result_.default_match()->keyword != last_default_keyword)));
+        (result_.default_match()->keyword != last_default_match->keyword)));
   if (notify_default_match)
     last_time_default_match_changed_ = base::TimeTicks::Now();
 

components/omnibox/browser/autocomplete_result.cc

@@ -37,6 +37,27 @@ using metrics::OmniboxEventProto;
 
 typedef AutocompleteMatchType ACMatchType;
 
+namespace {
+
+// Rotates |it| and its associated submatches to be in the front of |matches|.
+// |it| must be a valid iterator of |matches| or equal to |matches->end()|.
+void RotateMatchToFront(ACMatches::iterator it, ACMatches* matches) {
+  if (it == matches->end())
+    return;
+
+  const size_t cookie = it->subrelevance;
+  auto next = std::next(it);
+  if (cookie != 0) {
+    // If default match followed by sub-match(es), move them too.
+    while (next != matches->end() &&
+           AutocompleteMatch::IsSameFamily(cookie, next->subrelevance))
+      next = std::next(next);
+  }
+  std::rotate(matches->begin(), it, next);
+}
+
+}  // namespace
+
 struct MatchGURLHash {
   // The |bool| is whether the match is a calculator suggestion. We want them
   // compare differently against other matches with the same URL.
@@ -162,7 +183,8 @@ void AutocompleteResult::AppendMatches(const AutocompleteInput& input,
 
 void AutocompleteResult::SortAndCull(
     const AutocompleteInput& input,
-    TemplateURLService* template_url_service) {
+    TemplateURLService* template_url_service,
+    const AutocompleteMatch* preserve_default_match) {
   for (auto i(matches_.begin()); i != matches_.end(); ++i)
     i->ComputeStrippedDestinationURL(input, template_url_service);
 
@@ -186,19 +208,31 @@ void AutocompleteResult::SortAndCull(
   CompareWithDemoteByType<AutocompleteMatch> comparing_object(
       input.current_page_classification());
   std::sort(matches_.begin(), matches_.end(), comparing_object);
-  // Top match is not allowed to be the default match.  Find the most
-  // relevant legal match and shift it to the front.
-  auto it = FindTopMatch(input, &matches_);
-  if (it != matches_.end()) {
-    const size_t cookie = it->subrelevance;
-    auto next = std::next(it);
-    if (cookie != 0) {
-      // If default match followed by sub-match(es), move them too.
-      while (next != matches_.end() &&
-             AutocompleteMatch::IsSameFamily(cookie, next->subrelevance))
-        next = std::next(next);
+
+  // Find the best match and rotate it to the front to become the default match.
+  {
+    ACMatches::iterator top_match = matches_.end();
+
+    // If we are trying to keep a default match from a previous pass stable,
+    // search the current results for it, and if found, make it the top match.
+    if (preserve_default_match) {
+      std::pair<GURL, bool> default_match_fields =
+          GetMatchComparisonFields(*preserve_default_match);
+
+      top_match = std::find_if(
+          matches_.begin(), matches_.end(), [&](const AutocompleteMatch& m) {
+            // Find a match that is a duplicate AND has the same fill_into_edit.
+            return default_match_fields == GetMatchComparisonFields(m) &&
+                   preserve_default_match->fill_into_edit == m.fill_into_edit;
+          });
     }
-    std::rotate(matches_.begin(), it, next);
+
+    // Otherwise, if there's no default match from a previous pass to preserve,
+    // find the top match based on our normal undemoted scoring method.
+    if (top_match == matches_.end())
+      top_match = FindTopMatch(input, &matches_);
+
+    RotateMatchToFront(top_match, &matches_);
   }
 
   DiscourageTopMatchFromBeingSearchEntity(&matches_);

components/omnibox/browser/autocomplete_result.h

@@ -50,11 +50,18 @@ class AutocompleteResult {
 
   // Removes duplicates, puts the list in sorted order and culls to leave only
   // the best GetMaxMatches() matches. Sets the default match to the best match
-  // and updates the alternate nav URL. On desktop, it filters the matches to be
-  // either all tail suggestions (except for the first match) or no tail
-  // suggestions.
+  // and updates the alternate nav URL.
+  //
+  // |preserve_default_match| can be used to prevent the default match from
+  // being surprisingly swapped out during the asynchronous pass. If it has a
+  // value, this method searches the results for that match, and promotes it to
+  // the top. But we don't add back that match if it doesn't already exist.
+  //
+  // On desktop, it filters the matches to be either all tail suggestions
+  // (except for the first match) or no tail suggestions.
   void SortAndCull(const AutocompleteInput& input,
-                   TemplateURLService* template_url_service);
+                   TemplateURLService* template_url_service,
+                   const AutocompleteMatch* preserve_default_match = nullptr);
 
   // Creates and adds any dedicated Pedal matches triggered by existing matches.
   // This should be the only place where new Pedal suggestions are introduced

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -749,6 +749,47 @@ TEST_F(AutocompleteResultTest,
   }
 }
 
+// Test SortAndCull promoting a lower-scoring match to keep the default match
+// stable during the asynchronous pass.
+TEST_F(AutocompleteResultTest, SortAndCullWithPreserveDefaultMatch) {
+  TestData last[] = {
+      {0, 1, 500, true},
+      {1, 1, 400, true},
+  };
+  // Same as |last|, but with the scores swapped.
+  TestData current[] = {
+      {1, 1, 500, true},
+      {0, 1, 400, true},
+  };
+
+  AutocompleteInput input(base::ASCIIToUTF16("a"),
+                          metrics::OmniboxEventProto::OTHER,
+                          TestSchemeClassifier());
+
+  ACMatches last_matches;
+  PopulateAutocompleteMatches(last, base::size(last), &last_matches);
+  AutocompleteResult last_result;
+  last_result.AppendMatches(input, last_matches);
+  last_result.SortAndCull(input, template_url_service_.get());
+
+  ACMatches current_matches;
+  PopulateAutocompleteMatches(current, base::size(current), &current_matches);
+  AutocompleteResult current_result;
+  current_result.AppendMatches(input, current_matches);
+
+  // Run SortAndCull, but try to keep the first entry of last_matches on top.
+  current_result.SortAndCull(input, template_url_service_.get(),
+                             last_result.match_at(0));
+
+  // Assert that the lower scoring match has been promoted to the top to keep
+  // the default match stable.
+  TestData result[] = {
+      {0, 1, 400, true},
+      {1, 1, 500, true},
+  };
+  AssertResultMatches(current_result, result, base::size(result));
+}
+
 TEST_F(AutocompleteResultTest, DemoteOnDeviceSearchSuggestions) {
   // clang-format off
   TestData data[] = {