[omnibox] Refactor Autocomplete deduplication. Sort from best to worst.

This CL makes AutocompleteMatch::duplicate_matches sorted from
best-to-worst.

It refactors the deduplication process by splitting the duplicate
preference sorting code into a comparator, and puts the relevance
upgrading logic into its own method in:
AutocompleteMatch::UpgradeMatchWithPropertiesFrom().

This CL is needed, because almost any re-duplication logic used for
Entity Suggest "plain" match (see bug) will need to sort the duplicates
from second-best to worst.

But this should also be a pretty beneficial change on its own for code
clarity.

Bug: 989232
Change-Id: I71f093fe69714ee491328a54e3e2238a3ecadbbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730598
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686474}

components/omnibox/browser/autocomplete_match.cc

@@ -388,13 +388,57 @@ base::string16 AutocompleteMatch::GetWhyThisSuggestionText() const {
 }
 
 // static
-bool AutocompleteMatch::MoreRelevant(const AutocompleteMatch& elem1,
-                                     const AutocompleteMatch& elem2) {
+bool AutocompleteMatch::MoreRelevant(const AutocompleteMatch& match1,
+                                     const AutocompleteMatch& match2) {
   // For equal-relevance matches, we sort alphabetically, so that providers
   // who return multiple elements at the same priority get a "stable" sort
   // across multiple updates.
-  return (elem1.relevance == elem2.relevance) ?
-      (elem1.contents < elem2.contents) : (elem1.relevance > elem2.relevance);
+  return (match1.relevance == match2.relevance)
+             ? (match1.contents < match2.contents)
+             : (match1.relevance > match2.relevance);
+}
+
+// static
+bool AutocompleteMatch::BetterDuplicate(const AutocompleteMatch& match1,
+                                        const AutocompleteMatch& match2) {
+  // Prefer the Entity Match over the non-entity match, if they have the same
+  // |fill_into_edit| value.
+  if (match1.type == AutocompleteMatchType::SEARCH_SUGGEST_ENTITY &&
+      match2.type != AutocompleteMatchType::SEARCH_SUGGEST_ENTITY &&
+      match1.fill_into_edit == match2.fill_into_edit) {
+    return true;
+  }
+  if (match1.type != AutocompleteMatchType::SEARCH_SUGGEST_ENTITY &&
+      match2.type == AutocompleteMatchType::SEARCH_SUGGEST_ENTITY &&
+      match1.fill_into_edit == match2.fill_into_edit) {
+    return false;
+  }
+
+  // Prefer matches allowed to be the default match.
+  if (match1.allowed_to_be_default_match && !match2.allowed_to_be_default_match)
+    return true;
+  if (!match1.allowed_to_be_default_match && match2.allowed_to_be_default_match)
+    return false;
+
+  // Prefer document suggestions.
+  if (match1.type == AutocompleteMatchType::DOCUMENT_SUGGESTION &&
+      match2.type != AutocompleteMatchType::DOCUMENT_SUGGESTION) {
+    return true;
+  }
+  if (match1.type != AutocompleteMatchType::DOCUMENT_SUGGESTION &&
+      match2.type == AutocompleteMatchType::DOCUMENT_SUGGESTION) {
+    return false;
+  }
+
+  // By default, simply prefer the more relevant match.
+  return MoreRelevant(match1, match2);
+}
+
+// static
+bool AutocompleteMatch::BetterDuplicateByIterator(
+    const std::vector<AutocompleteMatch>::const_iterator it1,
+    const std::vector<AutocompleteMatch>::const_iterator it2) {
+  return BetterDuplicate(*it1, *it2);
 }
 
 // static
@@ -995,6 +1039,25 @@ bool AutocompleteMatch::ShouldShowButton() const {
   return ShouldShowTabMatch();
 }
 
+void AutocompleteMatch::UpgradeMatchWithPropertiesFrom(
+    const AutocompleteMatch& duplicate_match) {
+  // For Entity Matches, absorb the duplicate match's |allowed_to_be_default|
+  // and |inline_autocomplete| properties.
+  if (type == AutocompleteMatchType::SEARCH_SUGGEST_ENTITY &&
+      fill_into_edit == duplicate_match.fill_into_edit &&
+      duplicate_match.allowed_to_be_default_match) {
+    allowed_to_be_default_match = true;
+    if (inline_autocompletion.empty())
+      inline_autocompletion = duplicate_match.inline_autocompletion;
+  }
+
+  // And always absorb the higher relevance score of duplicates.
+  if (duplicate_match.relevance > relevance) {
+    RecordAdditionalInfo(kACMatchPropertyScoreBoostedFrom, relevance);
+    relevance = duplicate_match.relevance;
+  }
+}
+
 #if DCHECK_IS_ON()
 void AutocompleteMatch::Validate() const {
   ValidateClassifications(contents, contents_class);

components/omnibox/browser/autocomplete_match.h

@@ -170,8 +170,16 @@ struct AutocompleteMatch {
 
   // Comparison function for determining whether the first match is better than
   // the second.
-  static bool MoreRelevant(const AutocompleteMatch& elem1,
-                           const AutocompleteMatch& elem2);
+  static bool MoreRelevant(const AutocompleteMatch& match1,
+                           const AutocompleteMatch& match2);
+
+  // Comparison functions for determining whether the first match is preferred
+  // over the second when choosing between candidate duplicates.
+  static bool BetterDuplicate(const AutocompleteMatch& match1,
+                              const AutocompleteMatch& match2);
+  static bool BetterDuplicateByIterator(
+      const std::vector<AutocompleteMatch>::const_iterator it1,
+      const std::vector<AutocompleteMatch>::const_iterator it2);
 
   // Helper functions for classes creating matches:
   // Fills in the classifications for |text|, using |style| as the base style
@@ -424,6 +432,11 @@ struct AutocompleteMatch {
   // Returns true if the suggestion should show a tab match button or pedal.
   bool ShouldShowButton() const;
 
+  // Upgrades this match by absorbing the best properties from
+  // |duplicate_match|. For instance: if |duplicate_match| has a higher
+  // relevance score, this match's own relevance score will be upgraded.
+  void UpgradeMatchWithPropertiesFrom(const AutocompleteMatch& duplicate_match);
+
   // The provider of this match, used to remember which provider the user had
   // selected when the input changes. This may be NULL, in which case there is
   // no provider (or memory of the user's selection).
@@ -575,8 +588,11 @@ struct AutocompleteMatch {
   // property and associated value and which is presented in chrome://omnibox.
   AdditionalInfo additional_info;
 
-  // A list of matches culled during de-duplication process, retained to
-  // ensure if a match is deleted, the duplicates are deleted as well.
+  // A vector of matches culled during de-duplication process, sorted from
+  // second-best to worst according to the de-duplication preference criteria.
+  // This vector is retained so that if the user deletes a match, all the
+  // duplicates are deleted as well. This is also used for re-duping Search
+  // Entity vs. plain Search suggestions.
   std::vector<AutocompleteMatch> duplicate_matches;
 
   // So users of AutocompleteMatch can use the same ellipsis that it uses.

components/omnibox/browser/autocomplete_result.cc

@@ -9,6 +9,7 @@
 #include <iterator>
 #include <string>
 #include <unordered_set>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ -175,7 +176,7 @@ void AutocompleteResult::SortAndCull(
     MaybeCullTailSuggestions(&matches_);
   }
 #endif
-  SortAndDedupMatches(input.current_page_classification(), &matches_);
+  DeduplicateMatches(input.current_page_classification(), &matches_);
 
   DemoteOnDeviceSearchSuggestions();
 
@@ -565,11 +566,11 @@ GURL AutocompleteResult::ComputeAlternateNavUrl(
              : GURL();
 }
 
-void AutocompleteResult::SortAndDedupMatches(
+void AutocompleteResult::DeduplicateMatches(
     metrics::OmniboxEventProto::PageClassification page_classification,
     ACMatches* matches) {
   // Group matches by stripped URL and whether it's a calculator suggestion.
-  std::unordered_map<std::pair<GURL, bool>, std::list<ACMatches::iterator>,
+  std::unordered_map<std::pair<GURL, bool>, std::vector<ACMatches::iterator>,
                      MatchGURLHash>
       url_to_matches;
   for (auto i = matches->begin(); i != matches->end(); ++i) {
@@ -581,35 +582,39 @@ void AutocompleteResult::SortAndDedupMatches(
   for (auto& group : url_to_matches) {
     const auto& key = group.first;
     const GURL& gurl = key.first;
-    // The list of matches whose URL are equivalent.
-    std::list<ACMatches::iterator>& duplicate_matches = group.second;
+    // The vector of matches whose URL are equivalent.
+    std::vector<ACMatches::iterator>& duplicate_matches = group.second;
     if (gurl.is_empty() || duplicate_matches.size() == 1)
       continue;
 
-    // Find the best match.
-    auto best_match = duplicate_matches.begin();
-    for (auto i = std::next(best_match); i != duplicate_matches.end(); ++i) {
-      best_match = BetterDuplicate(i, best_match);
-    }
-
-    // Rotate the chosen match to be 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 each duplicate match, append its duplicates to that of the best
-    // match, then append it, before we erase it.
-    for (auto i = std::next(best_match); i != duplicate_matches.end(); ++i) {
-      auto& match = **i;
-      for (auto& dup_match : match.duplicate_matches)
-        (*best_match)->duplicate_matches.push_back(std::move(dup_match));
-      // Erase the duplicates before copying it. We don't need them any more.
-      match.duplicate_matches.erase(match.duplicate_matches.begin(),
-                                    match.duplicate_matches.end());
-      // Copy, don't move, because we need these below.
-      (*best_match)->duplicate_matches.push_back(match);
+    // Sort the matches best to worst, according to the deduplication criteria.
+    std::sort(duplicate_matches.begin(), duplicate_matches.end(),
+              &AutocompleteMatch::BetterDuplicateByIterator);
+    AutocompleteMatch& best_match = **duplicate_matches.begin();
+
+    // Process all the duplicate matches (from second-best to worst).
+    std::vector<AutocompleteMatch> duplicates_of_duplicates;
+    for (auto i = std::next(duplicate_matches.begin());
+         i != duplicate_matches.end(); ++i) {
+      AutocompleteMatch& duplicate_match = **i;
+
+      // Each duplicate match may also have its own duplicates. Move those to
+      // a temporary list, which will be eventually added to the end of
+      // |best_match.duplicate_matches|. Clear out the original list too.
+      std::move(duplicate_match.duplicate_matches.begin(),
+                duplicate_match.duplicate_matches.end(),
+                std::back_inserter(duplicates_of_duplicates));
+      duplicate_match.duplicate_matches.clear();
+
+      best_match.UpgradeMatchWithPropertiesFrom(duplicate_match);
+
+      // This should be a copy, not a move, since we don't erase duplicate
+      // matches from the master list until the very end.
+      DCHECK(duplicate_match.duplicate_matches.empty());  // Should be cleared.
+      best_match.duplicate_matches.push_back(duplicate_match);
     }
+    std::move(duplicates_of_duplicates.begin(), duplicates_of_duplicates.end(),
+              std::back_inserter(best_match.duplicate_matches));
   }
 
   // Erase duplicate matches.
@@ -650,91 +655,6 @@ size_t AutocompleteResult::EstimateMemoryUsage() const {
   return res;
 }
 
-// static
-std::list<ACMatches::iterator>::iterator AutocompleteResult::BetterDuplicate(
-    std::list<ACMatches::iterator>::iterator first,
-    std::list<ACMatches::iterator>::iterator second) {
-  std::list<ACMatches::iterator>::iterator preferred_match;
-  std::list<ACMatches::iterator>::iterator non_preferred_match;
-  // The following logic enforces constraints we care about regarding the
-  // the characteristics of the candidate matches. In order of priority:
-  //
-  // Entity suggestions:
-  //   Entity suggestions are always preferred over non-entity suggestions,
-  //   assuming both candidates have the same fill_into_edit value. In these
-  //   cases, because the fill_into_edit value is the same in both and the
-  //   selection of the entity suggestion appears to the user as simply a
-  //   "promotion" of an equivalent suggestion by adding additional decoration,
-  //   the entity suggestion is allowed to inherit the
-  //   allowed_to_be_default_match and inline_autocompletion values from the
-  //   other suggestion.
-  //
-  // allowed_to_be_default_match:
-  //   A suggestion that is allowed to be the default match is always preferred
-  //   over one that is not.
-  //
-  // Note that together these two constraints enforce an overall constraint,
-  // that if either candidate has allowed_to_be_default_match = true, the match
-  // which is preferred will always have allowed_to_be_default_match = true.
-  //
-  // Document suggestions:
-  //   The icon and display of document suggestions are preferred over
-  //   history, bookmark, etc. items. The actual URLs may be different, but
-  //   logically dedupe to the same entity to which we'll navigate.
-  if ((*first)->type == ACMatchType::SEARCH_SUGGEST_ENTITY &&
-      (*second)->type != ACMatchType::SEARCH_SUGGEST_ENTITY &&
-      (*first)->fill_into_edit == (*second)->fill_into_edit) {
-    preferred_match = first;
-    non_preferred_match = second;
-    if ((*non_preferred_match)->allowed_to_be_default_match) {
-      (*preferred_match)->allowed_to_be_default_match = true;
-      (*preferred_match)->inline_autocompletion =
-          (*non_preferred_match)->inline_autocompletion;
-    }
-  } else if ((*first)->type != ACMatchType::SEARCH_SUGGEST_ENTITY &&
-             (*second)->type == ACMatchType::SEARCH_SUGGEST_ENTITY &&
-             (*first)->fill_into_edit == (*second)->fill_into_edit) {
-    preferred_match = second;
-    non_preferred_match = first;
-    if ((*non_preferred_match)->allowed_to_be_default_match) {
-      (*preferred_match)->allowed_to_be_default_match = true;
-      (*preferred_match)->inline_autocompletion =
-          (*non_preferred_match)->inline_autocompletion;
-    }
-  } else if ((*first)->allowed_to_be_default_match &&
-             !(*second)->allowed_to_be_default_match) {
-    preferred_match = first;
-    non_preferred_match = second;
-  } else if ((*second)->allowed_to_be_default_match &&
-             !(*first)->allowed_to_be_default_match) {
-    preferred_match = second;
-    non_preferred_match = first;
-  } else if ((*first)->type == ACMatchType::DOCUMENT_SUGGESTION &&
-             (*second)->type != ACMatchType::DOCUMENT_SUGGESTION) {
-    preferred_match = first;
-    non_preferred_match = second;
-  } else if ((*first)->type != ACMatchType::DOCUMENT_SUGGESTION &&
-             (*second)->type == ACMatchType::DOCUMENT_SUGGESTION) {
-    preferred_match = second;
-    non_preferred_match = first;
-  } else {
-    // By default, simply prefer the match with the higher relevance. Note that
-    // we do not apply type-based demotion here (CompareWithDemoteByType)
-    // because we only apply demotion when ordering the final set of matches.
-    return (*first)->relevance >= (*second)->relevance ? first : second;
-  }
-
-  // If a match is preferred despite having a lower score, boost its score
-  // to that of the other match.
-  if ((*non_preferred_match)->relevance > (*preferred_match)->relevance) {
-    (*preferred_match)
-        ->RecordAdditionalInfo(kACMatchPropertyScoreBoostedFrom,
-                               (*preferred_match)->relevance);
-    (*preferred_match)->relevance = (*non_preferred_match)->relevance;
-  }
-  return preferred_match;
-}
-
 // static
 bool AutocompleteResult::HasMatchByDestination(const AutocompleteMatch& match,
                                                const ACMatches& matches) {

components/omnibox/browser/autocomplete_result.h

@@ -7,7 +7,6 @@
 
 #include <stddef.h>
 
-#include <list>
 #include <map>
 
 #include "base/macros.h"
@@ -155,23 +154,13 @@ class AutocompleteResult {
   typedef ACMatches::iterator::difference_type matches_difference_type;
 #endif
 
-  // Sort |matches| by destination, taking into account demotions based on
-  // |page_classification| when resolving ties about which of several
-  // duplicates to keep.  The matches are also deduplicated. Duplicate matches
-  // are stored in the |duplicate_matches| vector of the corresponding
-  // AutocompleteMatch.
-  static void SortAndDedupMatches(
+  // Modifies |matches| such that any duplicate matches are coalesced into
+  // representative "best" matches. The erased matches are moved into the
+  // |duplicate_matches| members of their representative matches.
+  static void DeduplicateMatches(
       metrics::OmniboxEventProto::PageClassification page_classification,
       ACMatches* matches);
 
-  // Examines |first| and |second| and returns the match that is preferred when
-  // choosing between candidate duplicates. Note that this may modify the
-  // relevance, allowed_to_be_default_match, or inline_autocompletion values of
-  // the returned match.
-  static std::list<ACMatches::iterator>::iterator BetterDuplicate(
-      std::list<ACMatches::iterator>::iterator first,
-      std::list<ACMatches::iterator>::iterator second);
-
   // Returns true if |matches| contains a match with the same destination as
   // |match|.
   static bool HasMatchByDestination(const AutocompleteMatch& match,

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -635,15 +635,15 @@ TEST_F(AutocompleteResultTest, SortAndCullWithMatchDups) {
   // Expect 3 unique results after SortAndCull().
   ASSERT_EQ(3U, result.size());
 
-  // Check that 3rd and 4th result got added to the first result as dups
+  // Check that 3rd and 4th result got added to the first result as duplicates
   // and also duplicates of the 4th match got copied.
   ASSERT_EQ(4U, result.match_at(0)->duplicate_matches.size());
   const AutocompleteMatch* first_match = result.match_at(0);
   EXPECT_EQ(matches[2].destination_url,
             first_match->duplicate_matches.at(1).destination_url);
-  EXPECT_EQ(dup_match.destination_url,
-            first_match->duplicate_matches.at(2).destination_url);
   EXPECT_EQ(matches[3].destination_url,
+            first_match->duplicate_matches.at(2).destination_url);
+  EXPECT_EQ(dup_match.destination_url,
             first_match->duplicate_matches.at(3).destination_url);
 
   // Check that 6th result started a new list of dups for the second result.
@@ -1765,8 +1765,8 @@ TEST_F(AutocompleteResultTest, PedalSuggestionsRemainUnique) {
 
   FakeAutocompleteProviderClient client;
   result.AppendDedicatedPedalMatches(&client, input);
-  result.SortAndDedupMatches(metrics::OmniboxEventProto::OTHER,
-                             &result.matches_);
+  result.DeduplicateMatches(metrics::OmniboxEventProto::OTHER,
+                            &result.matches_);
 
   // Exactly 2 (not 3) unique Pedals should be added with relevance close to max
   // of the triggering suggestions.
@@ -1781,8 +1781,8 @@ TEST_F(AutocompleteResultTest, PedalSuggestionsRemainUnique) {
   // no duplicates are added, but the existing Pedal suggestion is updated.
   result.match_at(3)->contents = base::UTF8ToUTF16("open incognito tab");
   result.AppendDedicatedPedalMatches(&client, input);
-  result.SortAndDedupMatches(metrics::OmniboxEventProto::OTHER,
-                             &result.matches_);
+  result.DeduplicateMatches(metrics::OmniboxEventProto::OTHER,
+                            &result.matches_);
   EXPECT_EQ(result.size(), 6u);
   EXPECT_NE(result.match_at(4)->pedal, nullptr);
   EXPECT_NE(result.match_at(5)->pedal, nullptr);

components/omnibox/browser/history_url_provider_unittest.cc

@@ -327,8 +327,8 @@ void HistoryURLProviderTest::RunTest(
     for (auto i = matches_.begin(); i != matches_.end(); ++i) {
       i->ComputeStrippedDestinationURL(input, service);
     }
-    AutocompleteResult::SortAndDedupMatches(input.current_page_classification(),
-                                            &matches_);
+    AutocompleteResult::DeduplicateMatches(input.current_page_classification(),
+                                           &matches_);
     std::sort(matches_.begin(), matches_.end(),
               &AutocompleteMatch::MoreRelevant);
   }

components/omnibox/browser/match_compare.h

@@ -6,22 +6,23 @@
 #define COMPONENTS_OMNIBOX_BROWSER_MATCH_COMPARE_H_
 
 #include "components/omnibox/browser/omnibox_field_trial.h"
+#include "third_party/metrics_proto/omnibox_event.pb.h"
+
+using PageClassification = metrics::OmniboxEventProto::PageClassification;
 
 // This class implements a special version of AutocompleteMatch::MoreRelevant
 // that allows matches of particular types to be demoted in AutocompleteResult.
 template <class Match>
 class CompareWithDemoteByType {
  public:
-  CompareWithDemoteByType(
-      metrics::OmniboxEventProto::PageClassification page_classification) {
+  CompareWithDemoteByType(PageClassification page_classification) {
     OmniboxFieldTrial::GetDemotionsByType(page_classification, &demotions_);
   }
 
   // Returns the relevance score of |match| demoted appropriately by
   // |demotions_by_type_|.
   int GetDemotedRelevance(const Match& match) const {
-    OmniboxFieldTrial::DemotionMultipliers::const_iterator demotion_it =
-        demotions_.find(match.type);
+    auto demotion_it = demotions_.find(match.type);
     return (demotion_it == demotions_.end())
                ? match.relevance
                : (match.relevance * demotion_it->second);
@@ -54,8 +55,7 @@ class CompareWithDemoteByType {
 template <class Match>
 class DestinationSort {
  public:
-  DestinationSort(
-      metrics::OmniboxEventProto::PageClassification page_classification)
+  DestinationSort(PageClassification page_classification)
       : demote_by_type_(page_classification) {}
   bool operator()(const Match& elem1, const Match& elem2) {
     // Sort identical destination_urls together.