Ensure the URL Matches do not sink below the Matches w/Headers.

This change rearranges the steps of the SortAndCull to ensure the
Matches with Headers are only grouped and demoted after the
GroupBySearchVsURL completes.

This update prevents the GroupBySearchVsURL from pushing the URLs
below the search suggestions, including the matches with group
headers.

Bug: 1165941
Change-Id: I70ea0815ab2c2c8b652368819c2eef1c40f692ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626188Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Cr-Commit-Position: refs/heads/master@{#845383}

components/omnibox/browser/autocomplete_result.cc

@@ -268,8 +268,6 @@ void AutocompleteResult::SortAndCull(
     LimitNumberOfURLsShown(GetMaxMatches(is_zero_suggest), max_url_count,
                            comparing_object);
 
-  GroupAndDemoteMatchesWithHeaders();
-
   // Limit total matches accounting for suggestions score <= 0, sub matches, and
   // feature configs such as OmniboxUIExperimentMaxAutocompleteMatches,
   // OmniboxMaxZeroSuggestMatches, and OmniboxDynamicMaxAutocomplete.
@@ -299,6 +297,12 @@ void AutocompleteResult::SortAndCull(
       BubbleURLSuggestions(next, begin_url, matches_);
   }
 
+  // Grouping and Demoting Matches with Headers needs to be done only after
+  // matches are grouped by Search and URL type to ensure that URLs don't sink
+  // to the bottom of the suggestions list, and surface below the Matches with
+  // headers.
+  GroupAndDemoteMatchesWithHeaders();
+
   // If we have a default match, run some sanity checks. Skip these checks if
   // the default match has no |destination_url|. An example of this is the
   // default match after the user has tabbed into keyword search mode, but has

components/omnibox/browser/autocomplete_result.h

@@ -238,6 +238,8 @@ class AutocompleteResult {
   FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
                            DemoteOnDeviceSearchSuggestions);
   FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest, BubbleURLSuggestions);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           SortAndCullKeepGroupedSuggestionsLast);
   friend class HistoryURLProviderTest;
 
   typedef std::map<AutocompleteProvider*, ACMatches> ProviderToMatches;

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -119,6 +119,9 @@ class AutocompleteResultTest : public testing::Test {
 
     // Type of the match
     AutocompleteMatchType::Type type{AutocompleteMatchType::SEARCH_SUGGEST};
+
+    // Suggestion Group ID for this suggeston
+    base::Optional<int> suggestion_group_id;
   };
 
   AutocompleteResultTest() {
@@ -201,6 +204,7 @@ void AutocompleteResultTest::PopulateAutocompleteMatch(
   match->relevance = data.relevance;
   match->allowed_to_be_default_match = data.allowed_to_be_default_match;
   match->duplicate_matches = data.duplicate_matches;
+  match->suggestion_group_id = data.suggestion_group_id;
 }
 
 void AutocompleteResultTest::PopulateAutocompleteMatches(
@@ -1563,6 +1567,52 @@ TEST_F(AutocompleteResultTest, SortAndCullGroupSuggestionsByType) {
 }
 #endif
 
+TEST_F(AutocompleteResultTest, SortAndCullKeepGroupedSuggestionsLast) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeaturesAndParameters(
+      {{omnibox::kUIExperimentMaxAutocompleteMatches,
+        {{OmniboxFieldTrial::kUIMaxAutocompleteMatchesParam, "6"}}}},
+      {/* nothing disabled */});
+  TestData data[] = {
+      {0, 1, 500, false, {}, AutocompleteMatchType::SEARCH_SUGGEST, 1},
+      {1, 2, 600, false, {}, AutocompleteMatchType::HISTORY_URL},
+      {2, 1, 700, false, {}, AutocompleteMatchType::SEARCH_SUGGEST, 1},
+      {3, 2, 800, true, {}, AutocompleteMatchType::HISTORY_TITLE},
+      {4, 1, 900, false, {}, AutocompleteMatchType::SEARCH_SUGGEST, 2},
+      {5, 2, 1000, false, {}, AutocompleteMatchType::SEARCH_SUGGEST, 2},
+      {6, 3, 1100, false, {}, AutocompleteMatchType::BOOKMARK_TITLE},
+  };
+  ACMatches matches;
+  PopulateAutocompleteMatches(data, base::size(data), &matches);
+
+  AutocompleteInput input(base::ASCIIToUTF16("a"),
+                          metrics::OmniboxEventProto::OTHER,
+                          TestSchemeClassifier());
+  AutocompleteResult result;
+
+  result.headers_map_[1] = STRING16_LITERAL("1");
+  result.headers_map_[2] = STRING16_LITERAL("2");
+
+  result.AppendMatches(input, matches);
+  result.SortAndCull(input, template_url_service_.get());
+
+  TestData expected_data[] = {
+      // default match unmoved
+      {3, 2, 800, true, {}, AutocompleteMatchType::HISTORY_TITLE},
+      // search types
+      {6, 3, 1100, false, {}, AutocompleteMatchType::BOOKMARK_TITLE},
+      {1, 2, 600, false, {}, AutocompleteMatchType::HISTORY_URL},
+      // Group <2> is scored higher
+      {5, 2, 1000, false, {}, AutocompleteMatchType::SEARCH_SUGGEST},
+      {4, 1, 900, false, {}, AutocompleteMatchType::SEARCH_SUGGEST},
+      // Group <1> is scored lower
+      {2, 1, 700, false, {}, AutocompleteMatchType::SEARCH_SUGGEST},
+  };
+
+  AssertResultMatches(result, expected_data,
+                      AutocompleteResult::GetMaxMatches());
+}
+
 TEST_F(AutocompleteResultTest, SortAndCullMaxURLMatches) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitWithFeaturesAndParameters(