[omnibox] Sort submatches consistently with parent match

Submatches normally have the same relevance as their parents
and are thus sorted positionally with them. However, if
there are demote-by-type rules in effect, a parent (or a
submatch) can have an altered relevance and thus be
sorted away from their family.

This CL records within each submatch the type of its parent
so that it can be sorted identically.

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

components/omnibox/browser/autocomplete_match.cc

@@ -142,6 +142,7 @@ AutocompleteMatch::AutocompleteMatch(const AutocompleteMatch& match)
       answer(match.answer),
       transition(match.transition),
       type(match.type),
+      parent_type(match.parent_type),
       has_tab_match(match.has_tab_match),
       subtype_identifier(match.subtype_identifier),
       associated_keyword(match.associated_keyword
@@ -194,6 +195,7 @@ AutocompleteMatch& AutocompleteMatch::operator=(
   answer = match.answer;
   transition = match.transition;
   type = match.type;
+  parent_type = match.parent_type;
   has_tab_match = match.has_tab_match;
   subtype_identifier = match.subtype_identifier;
   associated_keyword.reset(
@@ -607,6 +609,13 @@ bool AutocompleteMatch::IsSpecializedSearchType(Type type) {
          type == AutocompleteMatchType::SEARCH_SUGGEST_PROFILE;
 }
 
+AutocompleteMatch::Type AutocompleteMatch::GetDemotionType() const {
+  if (!IsSubMatch())
+    return type;
+  else
+    return parent_type;
+}
+
 // static
 TemplateURL* AutocompleteMatch::GetTemplateURLWithKeyword(
     TemplateURLService* template_url_service,
@@ -825,6 +834,12 @@ bool AutocompleteMatch::IsSameFamily(size_t lhs, size_t rhs) {
   return (lhs & FAMILY_SIZE_MASK) == (rhs & FAMILY_SIZE_MASK);
 }
 
+void AutocompleteMatch::SetSubMatch(size_t subrelevance,
+                                    AutocompleteMatch::Type parent_type) {
+  this->subrelevance = subrelevance;
+  this->parent_type = parent_type;
+}
+
 bool AutocompleteMatch::IsSubMatch() const {
   return subrelevance & ~FAMILY_SIZE_MASK;
 }
@@ -885,7 +900,7 @@ AutocompleteMatch AutocompleteMatch::DerivePedalSuggestion(
   copy.pedal = pedal;
   if (subrelevance == 0)
     subrelevance = GetNextFamilyID();
-  copy.subrelevance = subrelevance + PEDAL_FAMILY_ID;
+  copy.SetSubMatch(subrelevance + PEDAL_FAMILY_ID, copy.type);
   DCHECK(IsSameFamily(subrelevance, copy.subrelevance));
 
   copy.type = Type::PEDAL;

components/omnibox/browser/autocomplete_match.h

@@ -237,6 +237,10 @@ struct AutocompleteMatch {
   // like entity, personalized, profile or postfix.
   static bool IsSpecializedSearchType(Type type);
 
+  // If this match is a submatch, returns the parent's type, otherwise this
+  // match's type.
+  Type GetDemotionType() const;
+
   // A static version GetTemplateURL() that takes the match's keyword and
   // match's hostname as parameters.  In short, returns the TemplateURL
   // associated with |keyword| if it exists; otherwise returns the TemplateURL
@@ -320,6 +324,8 @@ struct AutocompleteMatch {
   // family, which will cause them to be sorted together.
   static size_t GetNextFamilyID();
   static bool IsSameFamily(size_t lhs, size_t rhs);
+  // Preferred method to set both fields simultaneously.
+  void SetSubMatch(size_t subrelevance, AutocompleteMatch::Type parent_type);
   bool IsSubMatch() const;
 
   // Computes the stripped destination URL (via GURLToStrippedGURL()) and
@@ -534,6 +540,9 @@ struct AutocompleteMatch {
   // Type of this match.
   Type type = AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED;
 
+  // If a submatch, the type of the parent.
+  Type parent_type;
+
   // True if we saw a tab that matched this suggestion.
   bool has_tab_match = false;
 

components/omnibox/browser/autocomplete_result.cc

@@ -792,7 +792,7 @@ void AutocompleteResult::LimitNumberOfURLsShown(
     const CompareWithDemoteByType<AutocompleteMatch>& comparing_object) {
   size_t search_count = std::count_if(
       matches_.begin(), matches_.end(), [&](const AutocompleteMatch& m) {
-        return AutocompleteMatch::IsSearchType(m.type) &&
+        return !m.IsSubMatch() && AutocompleteMatch::IsSearchType(m.type) &&
                // Don't count if would be removed.
                comparing_object.GetDemotedRelevance(m) > 0;
       });
@@ -801,16 +801,23 @@ void AutocompleteResult::LimitNumberOfURLsShown(
   if (GetMaxMatches() > search_count &&
       GetMaxMatches() - search_count > max_url_count)
     max_url_count = GetMaxMatches() - search_count;
-  size_t url_count = 0, total_count = 0;
+  size_t url_count = 0;
   // Erase URL suggestions past the count of allowed ones, or anything past
   // maximum.
   matches_.erase(
       std::remove_if(matches_.begin(), matches_.end(),
-                     [&url_count, max_url_count,
-                      &total_count](const AutocompleteMatch& m) {
-                       return (!AutocompleteMatch::IsSearchType(m.type) &&
-                               ++url_count > max_url_count) ||
-                              ++total_count > GetMaxMatches();
+                     [&url_count, max_url_count](const AutocompleteMatch& m) {
+                       if (!m.IsSubMatch() &&
+                           !AutocompleteMatch::IsSearchType(m.type) &&
+                           ++url_count > max_url_count)
+                         return true;
+                       // Do not count submatches towards URL total, but
+                       // drop them if parent was dropped.
+                       if (m.IsSubMatch() &&
+                           !AutocompleteMatch::IsSearchType(m.parent_type) &&
+                           url_count > max_url_count)
+                         return true;
+                       return false;
                      }),
       matches_.end());
 }
@@ -818,59 +825,7 @@ void AutocompleteResult::LimitNumberOfURLsShown(
 // static
 void AutocompleteResult::GroupSuggestionsBySearchVsURL(iterator begin,
                                                        iterator end) {
-  // The following routine implements a semi-stateful stable partition. It
-  // moves all search-type matches towards the beginning of the range, (and
-  // non-search-type towards the end) but has to keep any submatches of a
-  // match together with that match.
-  //
-  // It does this by iterating through the list, top to bottom, so that it
-  // can associate submatches with their match simply by position. It finds
-  // a section of matches to be moved down, then finds the following section
-  // of matches to be moved up:
-  //
-  // Search-type              (a search-type in the correct position, skipped)
-  // Navigation-type          <- begin
-  // submatch Search-type
-  // Navigation-type
-  // Search-type              <- mid
-  // submatch Navigation-type
-  // ...                      <- temp_end
-  //
-  // We then call rotate() with those 3 iterators, then repeat the search,
-  // starting with where the navigation-types landed.
-
-  // Find the first element to be moved downwards. Skip matches in correct
-  // position.
-  while (begin != end &&
-         (AutocompleteMatch::IsSearchType(begin->type) ||
-         // Any submatch present would belong to the previous search-type
-         // match.
-         begin->IsSubMatch())) {
-    begin = std::next(begin);
-  }
-  while (begin != end) {
-    // Find the last element (technically, the end of the range) to be moved
-    // downwards.
-    auto mid = begin;
-    while (mid != end &&
-           (!AutocompleteMatch::IsSearchType(mid->type) || mid->IsSubMatch())) {
-      mid = std::next(mid);
-    }
-    // Find the last element (technically, the end of the range) to be moved
-    // upwards.
-    auto temp_end = mid;
-    while (temp_end != end &&
-           (AutocompleteMatch::IsSearchType(temp_end->type) ||
-            temp_end->IsSubMatch())) {
-      temp_end = std::next(temp_end);
-    }
-    if (mid != end) {
-      std::rotate(begin, mid, temp_end);
-      // Advance |begin| iterator over the elements now in correct position.
-      // |begin += temp_end - mid;|
-      begin = std::next(begin, std::distance(mid, temp_end));
-    } else {
-      break;
-    }
-  }
+  std::stable_partition(begin, end, [](const AutocompleteMatch& match) {
+    return match.IsSearchType(match.GetDemotionType());
+  });
 }

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -1846,21 +1846,25 @@ TEST_F(AutocompleteResultTest, TestGroupSuggestionsBySearchVsURL) {
   matches[2].type = AutocompleteMatchType::SEARCH_SUGGEST;
   // It's submatch to move up with it.
   matches[3].type = AutocompleteMatchType::HISTORY_URL;
+  matches[3].parent_type = AutocompleteMatchType::SEARCH_SUGGEST;
   matches[3].subrelevance = 4 + 1;
   // A non-search-type to move down.
   matches[4].type = AutocompleteMatchType::HISTORY_URL;
   // It's submatch to move down with it.
   matches[5].type = AutocompleteMatchType::SEARCH_SUGGEST;
+  matches[5].parent_type = AutocompleteMatchType::HISTORY_URL;
   matches[5].subrelevance = 8 + 1;
   // A search-type to move up.
   matches[6].type = AutocompleteMatchType::SEARCH_SUGGEST;
   // It's submatch to move up with it.
   matches[7].type = AutocompleteMatchType::HISTORY_URL;
+  matches[7].parent_type = AutocompleteMatchType::SEARCH_SUGGEST;
   matches[7].subrelevance = 12 + 1;
   // A non-search-type to "move down" (really, to stay).
   matches[8].type = AutocompleteMatchType::HISTORY_URL;
   // It's submatch to move down with it.
   matches[9].type = AutocompleteMatchType::SEARCH_SUGGEST;
+  matches[9].parent_type = AutocompleteMatchType::HISTORY_URL;
   matches[9].subrelevance = 16 + 1;
 
   AutocompleteResult::GroupSuggestionsBySearchVsURL(matches.begin(),
@@ -1874,3 +1878,65 @@ TEST_F(AutocompleteResultTest, TestGroupSuggestionsBySearchVsURL) {
                 matches[i].IsSubMatch());
   }
 }
+
+TEST_F(AutocompleteResultTest, SortAndCullWithDemotedSubmatches) {
+  base::test::ScopedFeatureList feature_list;
+  // Disable overriding features to only test standard sorting.
+  feature_list.InitWithFeatures({},
+                                {omnibox::kOmniboxGroupSuggestionsBySearchVsUrl,
+                                 omnibox::kOmniboxPreserveDefaultMatchScore});
+
+  ACMatches matches;
+  const AutocompleteMatchTestData data[] = {
+      {"http://history-url/", AutocompleteMatchType::HISTORY_URL},
+      {"http://search-history-submatch1/",
+       AutocompleteMatchType::SEARCH_HISTORY},
+      {"http://search-what-you-typed/",
+       AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED},
+      {"http://history-title/", AutocompleteMatchType::HISTORY_TITLE},
+      {"http://search-history-submatch2/",
+       AutocompleteMatchType::SEARCH_HISTORY},
+      {"http://search-history/", AutocompleteMatchType::SEARCH_HISTORY},
+  };
+  PopulateAutocompleteMatchesFromTestData(data, base::size(data), &matches);
+
+  // Construct submatch relations.
+  matches[0].subrelevance = 4;
+  matches[1].SetSubMatch(4 + 1, AutocompleteMatchType::HISTORY_URL);
+  matches[1].relevance = matches[0].relevance;
+
+  matches[3].subrelevance = 8;
+  matches[4].SetSubMatch(8 + 1, AutocompleteMatchType::HISTORY_TITLE);
+  matches[4].relevance = matches[3].relevance;
+
+  // Add a rule demoting history-url and killing history-title.
+  {
+    std::map<std::string, std::string> params;
+    params[std::string(OmniboxFieldTrial::kDemoteByTypeRule) + ":3:*"] =
+        "1:50,7:100,2:0";  // 3 == HOME_PAGE
+    ASSERT_TRUE(variations::AssociateVariationParams(
+        OmniboxFieldTrial::kBundledExperimentFieldTrialName, "A", params));
+  }
+  base::FieldTrialList::CreateFieldTrial(
+      OmniboxFieldTrial::kBundledExperimentFieldTrialName, "A");
+
+  AutocompleteInput input(base::ASCIIToUTF16("a"), OmniboxEventProto::HOME_PAGE,
+                          TestSchemeClassifier());
+  AutocompleteResult result;
+  result.AppendMatches(input, matches);
+  result.SortAndCull(input, template_url_service_.get());
+
+  // Check the new ordering. The search-history submatch should follow
+  // the demoted history-url match, and the history-title results and
+  // its submatch should be omitted.
+  ASSERT_EQ(4u, result.size());
+  EXPECT_EQ("http://search-what-you-typed/",
+            result.match_at(0)->destination_url.spec());
+  EXPECT_EQ("http://search-history/",
+            result.match_at(1)->destination_url.spec());
+  EXPECT_EQ("http://history-url/", result.match_at(2)->destination_url.spec());
+  EXPECT_EQ("http://search-history-submatch1/",
+            result.match_at(3)->destination_url.spec());
+  EXPECT_TRUE(AutocompleteMatch::IsSameFamily(
+      result.match_at(2)->subrelevance, result.match_at(3)->subrelevance));
+}

components/omnibox/browser/match_compare.h

@@ -22,7 +22,7 @@ class CompareWithDemoteByType {
   // Returns the relevance score of |match| demoted appropriately by
   // |demotions_by_type_|.
   int GetDemotedRelevance(const Match& match) const {
-    auto demotion_it = demotions_.find(match.type);
+    auto demotion_it = demotions_.find(match.GetDemotionType());
     return (demotion_it == demotions_.end())
                ? match.relevance
                : (match.relevance * demotion_it->second);

components/omnibox/browser/shortcuts_provider.cc

@@ -74,6 +74,8 @@ struct ShortcutMatch {
   const ShortcutsDatabase::Shortcut* shortcut;
   base::string16 contents;
   AutocompleteMatch::Type type;
+
+  AutocompleteMatch::Type GetDemotionType() const { return type; }
 };
 
 // Sorts |matches| by destination, taking into account demotions based on