[omnibox] Better default match duplicate handling

Currently we just sort suggestions by relevance and keep the best of
several duplicates. This can remove allowed_to_be_default_match's.
This change keeps the best default match, if any, and in fact
promotes it to the best relevance of the non-default matches
(if it's better).

Bug: 542780
Change-Id: Ie53f10d421412c390259891290fa15a951a4a2e1
Reviewed-on: https://chromium-review.googlesource.com/1174860
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586033}

components/omnibox/browser/autocomplete_match.cc

@@ -282,13 +282,6 @@ bool AutocompleteMatch::MoreRelevant(const AutocompleteMatch& elem1,
       (elem1.contents < elem2.contents) : (elem1.relevance > elem2.relevance);
 }
 
-// static
-bool AutocompleteMatch::DestinationsEqual(const AutocompleteMatch& elem1,
-                                          const AutocompleteMatch& elem2) {
-  return !elem1.stripped_destination_url.is_empty() &&
-         (elem1.stripped_destination_url == elem2.stripped_destination_url);
-}
-
 // static
 void AutocompleteMatch::ClassifyMatchInString(
     const base::string16& find_text,

components/omnibox/browser/autocomplete_match.h

@@ -133,13 +133,6 @@ struct AutocompleteMatch {
   static bool MoreRelevant(const AutocompleteMatch& elem1,
                            const AutocompleteMatch& elem2);
 
-  // Comparison function for removing matches with duplicate destinations.
-  // Destinations are compared using |stripped_destination_url|.  Pairs of
-  // matches with empty destinations are treated as differing, since empty
-  // destinations are expected for non-navigable matches.
-  static bool DestinationsEqual(const AutocompleteMatch& elem1,
-                                const AutocompleteMatch& elem2);
-
   // Helper functions for classes creating matches:
   // Fills in the classifications for |text|, using |style| as the base style
   // and marking the first instance of |find_text| as a match.  (This match

components/omnibox/browser/autocomplete_match_unittest.cc

@@ -393,6 +393,6 @@ TEST(AutocompleteMatchTest, Duplicates) {
     m2.destination_url = GURL(cases[i].url2);
     m2.ComputeStrippedDestinationURL(input, nullptr);
     EXPECT_EQ(cases[i].expected_duplicate,
-              AutocompleteMatch::DestinationsEqual(m1, m2));
+              m1.stripped_destination_url == m2.stripped_destination_url);
   }
 }

components/omnibox/browser/autocomplete_result.cc

@@ -29,6 +29,10 @@
 #include "third_party/metrics_proto/omnibox_input_type.pb.h"
 #include "ui/base/l10n/l10n_util.h"
 
+// A match attribute when a default match's score has been boosted
+// with a higher scoring non-default match.
+static const char kScoreBoostedFrom[] = "score_boosted_from";
+
 // static
 size_t AutocompleteResult::GetMaxMatches() {
   constexpr size_t kDefaultMaxAutocompleteMatches = 6;
@@ -352,29 +356,67 @@ GURL AutocompleteResult::ComputeAlternateNavUrl(
 void AutocompleteResult::SortAndDedupMatches(
     metrics::OmniboxEventProto::PageClassification page_classification,
     ACMatches* matches) {
-  // Sort matches such that duplicate matches are consecutive.
-  std::sort(matches->begin(), matches->end(),
-            DestinationSort<AutocompleteMatch>(page_classification));
-
-  // Set duplicate_matches for the first match before erasing duplicate
-  // matches.
-  for (ACMatches::iterator i(matches->begin()); i != matches->end(); ++i) {
-    for (int j = 1; (i + j != matches->end()) &&
-                    AutocompleteMatch::DestinationsEqual(*i, *(i + j));
-         ++j) {
-      AutocompleteMatch& dup_match(*(i + j));
-      i->duplicate_matches.insert(i->duplicate_matches.end(),
-                                  dup_match.duplicate_matches.begin(),
-                                  dup_match.duplicate_matches.end());
-      dup_match.duplicate_matches.clear();
-      i->duplicate_matches.push_back(dup_match);
+  // Group matches by stripped URL.
+  std::unordered_map<GURL, std::list<ACMatches::iterator>, MatchGURLHash>
+      url_to_matches;
+  for (auto i = matches->begin(); i != matches->end(); ++i)
+    url_to_matches[i->stripped_destination_url].push_back(i);
+  CompareWithDemoteByType<AutocompleteMatch> compare_demote_by_type(
+      page_classification);
+  // Find best default, and non-default, match in each group.
+  for (auto& group : url_to_matches) {
+    const GURL& gurl = group.first;
+    // The list of matches whose URL are equivalent.
+    auto& duplicate_matches = group.second;
+    if (gurl.is_empty() || duplicate_matches.size() == 1)
+      continue;
+
+    auto best_match = duplicate_matches.end();
+    auto best_default = duplicate_matches.end();
+    for (auto i = duplicate_matches.begin(); i != duplicate_matches.end();
+         ++i) {
+      if ((*i)->allowed_to_be_default_match) {
+        if (best_default == duplicate_matches.end() ||
+            // This object implements greater than.
+            compare_demote_by_type(**i, **best_default)) {
+          best_default = i;
+        }
+      }
+      if (best_match == duplicate_matches.end() ||
+          compare_demote_by_type(**i, **best_match)) {
+        best_match = i;
+      }
+    }
+    if (best_match != best_default && best_default != duplicate_matches.end()) {
+      (*best_default)
+          ->RecordAdditionalInfo(kScoreBoostedFrom, (*best_default)->relevance);
+      (*best_default)->relevance = (*best_match)->relevance;
+      best_match = best_default;
+    }
+    // Rotate best first if necessary, so we know to keep it.
+    if (best_match != duplicate_matches.begin()) {
+      duplicate_matches.splice(duplicate_matches.begin(), duplicate_matches,
+                               best_match);
+    }
+    for (auto i = std::next(best_match); i != duplicate_matches.end(); ++i) {
+      // For each duplicate match, append its duplicates to that of the best
+      // match, then append it, before we erase it.
+      (*best_match)->duplicate_matches.insert(
+          (*best_match)->duplicate_matches.end(),
+          (*i)->duplicate_matches.begin(),
+          (*i)->duplicate_matches.end());
+      (*best_match)->duplicate_matches.push_back(**i);
     }
   }
-
   // Erase duplicate matches.
-  matches->erase(std::unique(matches->begin(), matches->end(),
-                             &AutocompleteMatch::DestinationsEqual),
-                 matches->end());
+  matches->erase(
+      std::remove_if(
+          matches->begin(), matches->end(),
+          [&url_to_matches](const AutocompleteMatch& m) {
+            return !m.stripped_destination_url.is_empty() &&
+                   &(*url_to_matches[m.stripped_destination_url].front()) != &m;
+          }),
+      matches->end());
 }
 
 void AutocompleteResult::InlineTailPrefixes() {

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -12,6 +12,7 @@
 
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -780,6 +781,112 @@ TEST_F(AutocompleteResultTest, SortAndCullReorderForDefaultMatch) {
   }
 }
 
+TEST_F(AutocompleteResultTest, SortAndCullPromoteDefaultMatch) {
+  TestData data[] = {
+    { 0, 1, 1300, false },
+    { 1, 1, 1200, false },
+    { 2, 2, 1100, false },
+    { 2, 3, 1000, false },
+    { 2, 4, 900, true }
+  };
+  TestSchemeClassifier test_scheme_classifier;
+
+  // Check that reorder swaps up a result, and promotes relevance,
+  // appropriately.
+  ACMatches matches;
+  PopulateAutocompleteMatches(data, base::size(data), &matches);
+  AutocompleteInput input(base::ASCIIToUTF16("a"),
+                          metrics::OmniboxEventProto::HOME_PAGE,
+                          test_scheme_classifier);
+  AutocompleteResult result;
+  result.AppendMatches(input, matches);
+  result.SortAndCull(input, template_url_service_.get());
+  ASSERT_EQ(3U, result.size());
+  EXPECT_EQ("http://c/", result.match_at(0)->destination_url.spec());
+  EXPECT_EQ(1100, result.match_at(0)->relevance);
+  EXPECT_EQ(GetProvider(4), result.match_at(0)->provider);
+  EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
+  EXPECT_EQ("http://b/", result.match_at(2)->destination_url.spec());
+}
+
+TEST_F(AutocompleteResultTest, SortAndCullPromoteUnconsecutiveMatches) {
+  TestData data[] = {
+    { 0, 1, 1300, false },
+    { 1, 1, 1200, true },
+    { 3, 2, 1100, false },
+    { 2, 1, 1000, false },
+    { 3, 3, 900, true },
+    { 4, 1, 800, false },
+    { 3, 4, 700, false },
+  };
+  TestSchemeClassifier test_scheme_classifier;
+
+  // Check that reorder swaps up a result, and promotes relevance,
+  // even for a default match that isn't the best.
+  ACMatches matches;
+  PopulateAutocompleteMatches(data, base::size(data), &matches);
+  AutocompleteInput input(base::ASCIIToUTF16("a"),
+                          metrics::OmniboxEventProto::HOME_PAGE,
+                          test_scheme_classifier);
+  AutocompleteResult result;
+  result.AppendMatches(input, matches);
+  result.SortAndCull(input, template_url_service_.get());
+  ASSERT_EQ(5U, result.size());
+  EXPECT_EQ("http://b/", result.match_at(0)->destination_url.spec());
+  EXPECT_EQ(1200, result.match_at(0)->relevance);
+  EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
+  EXPECT_EQ("http://d/", result.match_at(2)->destination_url.spec());
+  EXPECT_EQ(1100, result.match_at(2)->relevance);
+  EXPECT_EQ(GetProvider(3), result.match_at(2)->provider);
+  EXPECT_EQ("http://c/", result.match_at(3)->destination_url.spec());
+  EXPECT_EQ("http://e/", result.match_at(4)->destination_url.spec());
+}
+
+TEST_F(AutocompleteResultTest, SortAndCullPromoteDuplicateSearchURLs) {
+  // Register a template URL that corresponds to 'foo' search engine.
+  TemplateURLData url_data;
+  url_data.SetShortName(base::ASCIIToUTF16("unittest"));
+  url_data.SetKeyword(base::ASCIIToUTF16("foo"));
+  url_data.SetURL("http://www.foo.com/s?q={searchTerms}");
+  template_url_service_->Add(std::make_unique<TemplateURL>(url_data));
+
+  TestData data[] = {
+    { 0, 1, 1300, false },
+    { 1, 1, 1200, true },
+    { 2, 1, 1100, true },
+    { 3, 1, 1000, true },
+    { 4, 2, 900,  true },
+  };
+
+  ACMatches matches;
+  PopulateAutocompleteMatches(data, base::size(data), &matches);
+  // Note that 0, 2 and 3 will compare equal after stripping.
+  matches[0].destination_url = GURL("http://www.foo.com/s?q=foo");
+  matches[1].destination_url = GURL("http://www.foo.com/s?q=foo2");
+  matches[2].destination_url = GURL("http://www.foo.com/s?q=foo&oq=f");
+  matches[3].destination_url = GURL("http://www.foo.com/s?q=foo&aqs=0");
+  matches[4].destination_url = GURL("http://www.foo.com/");
+
+  AutocompleteInput input(base::ASCIIToUTF16("a"),
+                          metrics::OmniboxEventProto::OTHER,
+                          TestSchemeClassifier());
+  AutocompleteResult result;
+  result.AppendMatches(input, matches);
+  result.SortAndCull(input, template_url_service_.get());
+
+  // We expect the 3rd and 4th results to be removed.
+  ASSERT_EQ(3U, result.size());
+  EXPECT_EQ("http://www.foo.com/s?q=foo&oq=f",
+            result.match_at(0)->destination_url.spec());
+  EXPECT_EQ(1300, result.match_at(0)->relevance);
+  EXPECT_EQ("http://www.foo.com/s?q=foo2",
+            result.match_at(1)->destination_url.spec());
+  EXPECT_EQ(1200, result.match_at(1)->relevance);
+  EXPECT_EQ("http://www.foo.com/",
+            result.match_at(2)->destination_url.spec());
+  EXPECT_EQ(900, result.match_at(2)->relevance);
+}
+
 TEST_F(AutocompleteResultTest, TopMatchIsStandaloneVerbatimMatch) {
   ACMatches matches;
   AutocompleteResult result;

components/omnibox/browser/match_compare.h

@@ -64,4 +64,10 @@ class DestinationSort {
   CompareWithDemoteByType<Match> demote_by_type_;
 };
 
+struct MatchGURLHash {
+  size_t operator()(const GURL& gurl) const {
+    return std::hash<std::string>()(gurl.spec());
+  }
+};
+
 #endif  // COMPONENTS_OMNIBOX_BROWSER_MATCH_COMPARE_H_