[Omnibox] Prevent copied-from-answer match from being deduped

Currently, this copy is de-duped with the answer suggestion in variants
where they occupy the same group (actually just treatment 1 in
practice). This is because they share a stripped destination url. It's
also possible for the answer suggestion to be de-duped with the verbatim
match, which copies its actions (but not answer data). We can make both
of these impossible by mutating the field used for deduping.

Also, sort the answer suggestion to the bottom when it doesn't occupy
its own group.

Bug: 326368632
Change-Id: I56ae15b5a1ca6db6637e8c4aa2bf240877ffbfa0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5685335
Commit-Queue: Patrick Noland <[email protected]>
Reviewed-by: Tomasz Wiszkowski <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1326359}

chrome/browser/autocomplete/search_provider_unittest.cc

@@ -4114,7 +4114,9 @@ TEST_F(SearchProviderTest, DuplicateCardAnswer) {
   AutocompleteMatch match1, match2, match3;
   match1.contents = u"match 1";
   match1.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  match1.allowed_to_be_default_match = true;
   match1.answer_template = omnibox::RichAnswerTemplate();
+  match1.destination_url = GURL("http://www.google.com/google.com/search?");
 
   matches.push_back(match1);
   matches.push_back(match2);
@@ -4124,7 +4126,10 @@ TEST_F(SearchProviderTest, DuplicateCardAnswer) {
 
   EXPECT_EQ(4u, matches.size());
   EXPECT_TRUE(matches[0].answer_template);
+  EXPECT_FALSE(matches[0].allowed_to_be_default_match);
   EXPECT_FALSE(matches[3].answer_template);
+  EXPECT_TRUE(matches[3].allowed_to_be_default_match);
+  EXPECT_EQ(matches[3].suggestion_group_id, omnibox::GROUP_SEARCH);
   EXPECT_EQ(matches[0].contents, matches[3].contents);
   EXPECT_EQ(matches[0].type, matches[3].type);
 }

components/omnibox/browser/autocomplete_match.cc

@@ -183,17 +183,22 @@ inline void hash_combine(std::size_t& seed, const T& value) {
 
 }  // namespace
 
-template <typename S, typename T>
-size_t ACMatchKeyHash<S, T>::operator()(const ACMatchKey<S, T>& key) const {
+template <typename... Args>
+size_t ACMatchKeyHash<Args...>::operator()(
+    const ACMatchKey<Args...>& key) const {
   size_t seed = 0;
-  hash_combine(seed, key.first);
-  hash_combine(seed, key.second);
+  // Compute a hash by applying hash_combine to each element of the "key" tuple.
+  std::apply([&seed](auto&&... args) { ((hash_combine(seed, args)), ...); },
+             key);
   return seed;
 }
 
 // This trick allows implementing ACMatchKeyHash in the implementation file.
-template struct ACMatchKeyHash<std::u16string, std::string>;
-template struct ACMatchKeyHash<std::string, bool>;
+// Every unique specialization of ACMatchKey should have a corresponding
+// declaration here.
+template struct ACMatchKeyHash<std::u16string,
+                               std::string>;  // base_search_provider
+template struct ACMatchKeyHash<std::string, bool, bool>;  // autocomplete_result
 
 // RichAutocompletionParams ---------------------------------------------------
 
@@ -1448,6 +1453,10 @@ int AutocompleteMatch::GetSortingOrder() const {
     return 3;
   }
 #endif  // !BUILDFLAG(IS_IOS)
+  if (answer_template && actions.size() > 0 &&
+      OmniboxFieldTrial::kAnswerActionsShowAboveKeyboard.Get()) {
+    return 4;
+  }
   if (IsSearchType(type)) {
     return 3;
   }

components/omnibox/browser/autocomplete_match.h

@@ -979,14 +979,14 @@ typedef std::vector<ACMatchClassification> ACMatchClassifications;
 typedef std::vector<AutocompleteMatch> ACMatches;
 
 // Can be used as the key for grouping AutocompleteMatches in a map based on a
-// std::pair of fields. This can be generalized to a std::tuple if ever needed.
+// std::tuple of fields.
 // The accompanying hash function makes the key usable in an std::unordered_map.
-template <typename S, typename T>
-using ACMatchKey = std::pair<S, T>;
+template <typename... Args>
+using ACMatchKey = std::tuple<Args...>;
 
-template <typename S, typename T>
+template <typename... Args>
 struct ACMatchKeyHash {
-  size_t operator()(const ACMatchKey<S, T>& key) const;
+  size_t operator()(const ACMatchKey<Args...>& key) const;
 };
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_MATCH_H_

components/omnibox/browser/autocomplete_result.cc

@@ -1154,7 +1154,7 @@ void AutocompleteResult::DeduplicateMatches(
   // Group matches by stripped URL and whether it's a calculator suggestion.
   std::unordered_map<AutocompleteResult::MatchDedupComparator,
                      std::vector<ACMatches::iterator>,
-                     ACMatchKeyHash<std::string, bool>>
+                     ACMatchKeyHash<std::string, bool, bool>>
       url_to_matches;
   for (auto i = matches->begin(); i != matches->end(); ++i) {
     url_to_matches[GetMatchComparisonFields(*i)].push_back(i);
@@ -1166,8 +1166,9 @@ void AutocompleteResult::DeduplicateMatches(
 
     // The vector of matches whose URL are equivalent.
     std::vector<ACMatches::iterator>& duplicate_matches = group.second;
-    if (key.first.empty() || duplicate_matches.size() == 1)
+    if (std::get<0>(key).empty() || duplicate_matches.size() == 1) {
       continue;
+    }
 
     // Sort the matches best to worst, according to the deduplication criteria.
     std::sort(duplicate_matches.begin(), duplicate_matches.end(),
@@ -1460,8 +1461,11 @@ void AutocompleteResult::MergeMatchesByProvider(ACMatches* old_matches,
 
 AutocompleteResult::MatchDedupComparator
 AutocompleteResult::GetMatchComparisonFields(const AutocompleteMatch& match) {
-  return std::make_pair(match.stripped_destination_url.spec(),
-                        match.type == ACMatchType::CALCULATOR);
+  return std::make_tuple(
+      match.stripped_destination_url.spec(),
+      match.type == ACMatchType::CALCULATOR,
+      match.answer_template.has_value() &&
+          OmniboxFieldTrial::kAnswerActionsShowAboveKeyboard.Get());
 }
 
 void AutocompleteResult::LimitNumberOfURLsShown(

components/omnibox/browser/autocomplete_result.h

@@ -39,7 +39,7 @@ class AutocompleteResult {
  public:
   typedef ACMatches::const_iterator const_iterator;
   typedef ACMatches::iterator iterator;
-  using MatchDedupComparator = ACMatchKey<std::string, bool>;
+  using MatchDedupComparator = ACMatchKey<std::string, bool, bool>;
 
   // Max number of matches we'll show from the various providers. This limit
   // may be different for zero suggest and non zero suggest. Does not take into
@@ -413,11 +413,8 @@ class AutocompleteResult {
   void MergeMatchesByProvider(ACMatches* old_matches,
                               const ACMatches& new_matches);
 
-  // This pulls the relevant fields out of a match for comparison with other
-  // matches for the purpose of deduping. It uses the stripped URL, so that we
-  // collapse similar URLs if necessary, and whether the match is a calculator
-  // suggestion, because we don't want to dedupe them against URLs that simply
-  // happen to go to the same destination.
+  // Returns a tuple encompassing all attributes relevant to determining whether a match should
+  // be deduplicated with another match.
   static MatchDedupComparator GetMatchComparisonFields(
       const AutocompleteMatch& match);
 

components/omnibox/browser/base_search_provider.cc

@@ -544,8 +544,8 @@ void BaseSearchProvider::AddMatchToMap(
   // Try to add `match` to `map`.
   // NOTE: Keep this ToLower() call in sync with url_database.cc.
   MatchKey match_key(
-      std::make_pair(base::i18n::ToLower(result.suggestion()),
-                     match.search_terms_args->additional_query_params));
+      std::make_tuple(base::i18n::ToLower(result.suggestion()),
+                      match.search_terms_args->additional_query_params));
   const std::pair<MatchMap::iterator, bool> i(
       map->insert(std::make_pair(match_key, match)));
   if (i.second) {
@@ -562,12 +562,12 @@ void BaseSearchProvider::AddMatchToMap(
     // Note that the match previously added to the map will continue to get the
     // typical `stripped_destination_url` allowing it to be deduped with the
     // plain-text matches (i.e., with no additional query params) as expected.
-    const auto& added_match_query = match_key.first;
-    const auto& added_match_query_params = match_key.second;
+    const auto& added_match_query = std::get<0>(match_key);
+    const auto& added_match_query_params = std::get<1>(match_key);
     if (!added_match_query_params.empty()) {
       for (const auto& entry : *map) {
-        const auto& existing_match_query = entry.first.first;
-        const auto& existing_match_query_params = entry.first.second;
+        const auto& existing_match_query = std::get<0>(entry.first);
+        const auto& existing_match_query_params = std::get<1>(entry.first);
         if (existing_match_query == added_match_query &&
             !existing_match_query_params.empty() &&
             existing_match_query_params != added_match_query_params) {

components/omnibox/browser/search_provider.cc

@@ -1139,7 +1139,14 @@ void SearchProvider::DuplicateCardAnswer(ACMatches* matches) {
     return;
   }
 
-  matches->emplace_back(*iter).answer_template.reset();
+  bool orig_allowed_to_be_default_match = iter->allowed_to_be_default_match;
+  iter->allowed_to_be_default_match = false;
+
+  auto& copy = matches->emplace_back(*iter);
+  copy.answer_template.reset();
+  copy.actions.clear();
+  copy.allowed_to_be_default_match = orig_allowed_to_be_default_match;
+  copy.suggestion_group_id = omnibox::GROUP_SEARCH;
 }
 
 bool SearchProvider::IsTopMatchSearchWithURLInput() const {