[omnibox] Group and demote matches with headers before they are culled

Moves the logic to demote autocomplete matches with headers to ACResult
where it is called after matches are deduped and sorted and before they
are culled. This step needs to happen before matches are culled so that
matches with higher relevance (remote ZPS) can be used to backfill
matches with lower relevance (local ZPS) before the lower relevance
ones are thrown away. The latter is a requirement for the Trending
suggestions experiment.

Also addresses a technical debt by grouping the matches with headers
in addition to demoting them to avoid interleaving of groups.

Bug: 1122669
Change-Id: Iba9fb9d02bc7ddd07805cfd31e32a84604da077c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386839
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803591}

components/omnibox/browser/autocomplete_controller.cc

@@ -685,6 +685,8 @@ void AutocompleteController::UpdateResult(
     result_.ConvertInSuggestionPedalMatches(provider_client_.get());
   }
 
+  UpdateHeaderInfoFromZeroSuggestProvider(&result_);
+
   // Sort the matches and trim to a small number of "best" matches.
   const AutocompleteMatch* preserve_default_match = nullptr;
   if (!in_start_ && last_default_match &&
@@ -713,7 +715,6 @@ void AutocompleteController::UpdateResult(
                                                      result_);
   }
 
-  UpdateHeaders(&result_);
   UpdateKeywordDescriptions(&result_);
   UpdateAssociatedKeywords(&result_);
   UpdateAssistedQueryStats(&result_);
@@ -802,25 +803,24 @@ void AutocompleteController::UpdateAssociatedKeywords(
   }
 }
 
-void AutocompleteController::UpdateHeaders(AutocompleteResult* result) {
-  DCHECK(result);
-
+void AutocompleteController::UpdateHeaderInfoFromZeroSuggestProvider(
+    AutocompleteResult* result) {
   // Currently, we only populate the AutocompleteResult's header labels from
   // ZeroSuggestProvider. Even if another provider has header metadata, we
   // currently ignore it. This means that as-you-type suggestions will NEVER
   // show headers in the UI. For now, this is hacky, but intended.
   //
   // TODO(tommycli): Stop special casing ZeroSuggestProvider here.
-  if (zero_suggest_provider_) {
-    result->set_headers_map(zero_suggest_provider_->headers_map());
-    result->set_hidden_group_ids(zero_suggest_provider_->hidden_group_ids());
-  }
+  if (!zero_suggest_provider_)
+    return;
+
+  result->set_headers_map(zero_suggest_provider_->headers_map());
+  result->set_hidden_group_ids(zero_suggest_provider_->hidden_group_ids());
 
   for (AutocompleteMatch& match : *result) {
     if (match.suggestion_group_id.has_value()) {
       // Record header data into the additional_info field for chrome://omnibox.
-      // Note, to improve debugging, we record the original group ID sent by
-      // the server before stripping empty headers.
+      // For improved debugging, we record all group IDs sent by the server.
       int group_id = match.suggestion_group_id.value();
       match.RecordAdditionalInfo("suggestion_group_id", group_id);
 
@@ -835,17 +835,6 @@ void AutocompleteController::UpdateHeaders(AutocompleteResult* result) {
       }
     }
   }
-
-  // Move all grouped matches to the bottom while maintaining the current order.
-  //
-  // TODO(tommycli): Currently, this pushes all suggestions with group IDs to
-  // the bottom, but doesn't group them together. That could lead to some
-  // awkward interleaving of groups.
-  std::stable_sort(result->begin(), result->end(),
-                   [](const auto& a, const auto& b) {
-                     return !a.suggestion_group_id.has_value() &&
-                            b.suggestion_group_id.has_value();
-                   });
 }
 
 void AutocompleteController::UpdateKeywordDescriptions(

components/omnibox/browser/autocomplete_controller.h

@@ -228,18 +228,14 @@ class AutocompleteController : public AutocompleteProviderListener,
   // relevance before this is called.
   void UpdateAssociatedKeywords(AutocompleteResult* result);
 
-  // Called for zero-prefix suggestions only.
-  // - Updates |result| with suggestion group ID to header mapping information.
-  // - Ensures matches that belong to a group appear at the bottom.
-  // Remote zero-prefix suggestions may be backfilled with local zero-prefix
-  // suggestions if there are not enough of them to fill all the available
-  // slots. However this cannot be done when remote reactive zero-prefix
-  // suggestions (aka rZPS) are present (i.e., there are suggestions with a
-  // |suggestion_groupd_id|), as those must appear under a header for
-  // transparency reasons. Hence we demote grouped matches to the bottom here.
-  // This function makes an implicit assumption that remote non-rZPS are not
-  // grouped. Otherwise local ZPS would appear at the top of the list.
-  void UpdateHeaders(AutocompleteResult* result);
+  // Updates |result| with the suggestion group ID to header string mapping as
+  // well as the set of hidden suggestion group IDs. Also, strips all match
+  // group IDs that don't have a equivalent header string; essentially treating
+  // those matches as ordinary ones that do not belong to any suggestion group.
+  // Called for zero-prefix suggestions only. This call is followed by
+  // AutocompleteResult::GroupAndDemoteMatchesWithHeaders() which groups and
+  // demotes matches with suggestion group IDs to the bottom of the result set.
+  void UpdateHeaderInfoFromZeroSuggestProvider(AutocompleteResult* result);
 
   // For each group of contiguous matches from the same TemplateURL, show the
   // provider name as a description on the first match in the group.

components/omnibox/browser/autocomplete_provider_unittest.cc

@@ -550,8 +550,10 @@ void AutocompleteProviderTest::UpdateResultsWithHeaderTestData(
   result_.Reset();
   result_.AppendMatches(AutocompleteInput(), matches);
 
-  // Update the result with the header information and demote grouped matches.
-  controller_->UpdateHeaders(&result_);
+  // Update the result with the header information.
+  controller_->UpdateHeaderInfoFromZeroSuggestProvider(&result_);
+  // Group matches with headers and move them to the bottom of the result set.
+  result_.GroupAndDemoteMatchesWithHeaders();
 }
 
 void AutocompleteProviderTest::RunAssistedQueryStatsTest(
@@ -775,7 +777,7 @@ TEST_F(AutocompleteProviderTest, ExactMatchKeywords) {
 }
 
 // Tests that the AutocompleteResult is updated with the header information and
-// grouped matches are demoted correctly.
+// matches with headers are grouped and demoted correctly.
 TEST_F(AutocompleteProviderTest, Headers) {
   ResetControllerWithKeywordAndSearchProviders();
 
@@ -820,22 +822,22 @@ TEST_F(AutocompleteProviderTest, Headers) {
                                 {
                                     {base::nullopt},
                                     {kRecentSearchesGroupId},
-                                    {kRecommendedForYouGroupId},
-                                    {base::nullopt},
                                     {base::nullopt},
+                                    {kRecommendedForYouGroupId},
+                                    {kRecentSearchesGroupId},
                                 }};
     UpdateResultsWithHeaderTestData(test_data);
 
-    // Verifies that matches with group IDs sink to the bottom.
+    // Verifies that matches with group IDs are grouped and sink to the bottom.
     EXPECT_FALSE(result_.match_at(0)->suggestion_group_id.has_value());
     EXPECT_FALSE(result_.match_at(1)->suggestion_group_id.has_value());
-    EXPECT_FALSE(result_.match_at(2)->suggestion_group_id.has_value());
+    EXPECT_EQ(kRecentSearchesGroupId,
+              result_.match_at(2)->suggestion_group_id.value());
     EXPECT_EQ(kRecentSearchesGroupId,
               result_.match_at(3)->suggestion_group_id.value());
     EXPECT_EQ(kRecommendedForYouGroupId,
               result_.match_at(4)->suggestion_group_id.value());
   }
-
   {
     HeaderTestData test_data = {headers_map,
                                 {{kGroupIdWithoutHeaderText},

components/omnibox/browser/autocomplete_result.cc

@@ -265,6 +265,8 @@ 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.
@@ -323,6 +325,49 @@ void AutocompleteResult::SortAndCull(
   }
 }
 
+void AutocompleteResult::GroupAndDemoteMatchesWithHeaders() {
+  constexpr int kNoHeaderSuggesetionGroupId = -1;
+
+  // Create a map from suggestion group ID to the index it first appears.
+  // Reserve the first spot for matches without headers.
+  std::map<int, int> group_id_index_map = {{kNoHeaderSuggesetionGroupId, 0}};
+  for (auto it = matches_.begin(); it != matches_.end(); ++it) {
+    if (it->suggestion_group_id.has_value()) {
+      // Make sure every suggestion group ID has an equivalent header string.
+      // AutocompleteController::UpdateHeaderInfoFromZeroSuggestProvider() is
+      // expected to always have be called before this function.
+      DCHECK(!GetHeaderForGroupId(it->suggestion_group_id.value()).empty());
+    }
+
+    int group_id =
+        it->suggestion_group_id.value_or(kNoHeaderSuggesetionGroupId);
+    // Use the 1-based index of the match to record the first appearance of its
+    // group ID since 0 is reserved for matches without headers. We are
+    // interested in the relative values of these indices only and their
+    // absolute values hardly matter.
+    int index = std::distance(matches_.begin(), it) + 1;
+    // map::insert doesn't insert the value if the map already contains the key.
+    group_id_index_map.insert(std::pair<int, int>(group_id, index));
+  }
+
+  // No need to group and demote matches with headers if none exists.
+  if (group_id_index_map.size() == 1)
+    return;
+
+  // Sort the matches based on the order in which their group IDs first appear
+  // while preserving the existing order of matches with the same group ID.
+  std::stable_sort(
+      matches_.begin(), matches_.end(),
+      [&group_id_index_map, kNoHeaderSuggesetionGroupId](const auto& a,
+                                                         const auto& b) {
+        const int a_group_id =
+            a.suggestion_group_id.value_or(kNoHeaderSuggesetionGroupId);
+        const int b_group_id =
+            b.suggestion_group_id.value_or(kNoHeaderSuggesetionGroupId);
+        return group_id_index_map[a_group_id] < group_id_index_map[b_group_id];
+      });
+}
+
 void AutocompleteResult::DemoteOnDeviceSearchSuggestions() {
   const std::string mode = OmniboxFieldTrial::OnDeviceHeadSuggestDemoteMode();
   if (mode != "decrease-relevances" && mode != "remove-suggestions")

components/omnibox/browser/autocomplete_result.h

@@ -73,6 +73,20 @@ class AutocompleteResult {
                    TemplateURLService* template_url_service,
                    const AutocompleteMatch* preserve_default_match = nullptr);
 
+  // Ensures that matches with headers, i.e., matches with a suggestion_group_id
+  // value, are grouped together at the bottom of result set based on their
+  // suggestion_group_id values and in the order the group IDs first appear.
+  // Certain types of remote zero-prefix matches need to appear under a header
+  // for transparency reasons. This information is sent to Chrome by the server.
+  // Also it is possible for zero-prefix matches from different providers (e.g.,
+  // local and remote) to mix and match. Hence, we group matches with the same
+  // headers and demote them to the bottom of the result set to ensure, one,
+  // matches without headers appear at the top of the result set, and two, there
+  // are no interleaving headers whether this is caused by bad server data or by
+  // mixing of local and remote zero-prefix suggestions.
+  // Called after matches are deduped and sorted and before they are culled.
+  void GroupAndDemoteMatchesWithHeaders();
+
   // Sets |pedal| in matches that have Pedal-triggering text.
   void ConvertInSuggestionPedalMatches(AutocompleteProviderClient* client);
 

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -244,6 +244,7 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
                           metrics::OmniboxEventProto::NTP,
                           TestSchemeClassifier());
   result->AppendMatches(input, matches);
+  result->set_headers_map({{7, base::UTF8ToUTF16("header")}});
   result->SortAndCull(input, nullptr);
   popup_model()->OnResultChanged();
   EXPECT_EQ(0u, model()->popup_model()->selected_line());
@@ -325,6 +326,7 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelectionWithHiddenGroupIds) {
                           metrics::OmniboxEventProto::NTP,
                           TestSchemeClassifier());
   result->AppendMatches(input, matches);
+  result->set_headers_map({{7, base::UTF8ToUTF16("header")}});
   result->SortAndCull(input, nullptr);
   popup_model()->OnResultChanged();
   EXPECT_EQ(0u, model()->popup_model()->selected_line());
@@ -404,6 +406,7 @@ TEST_F(OmniboxPopupModelSuggestionButtonRowTest,
                           metrics::OmniboxEventProto::NTP,
                           TestSchemeClassifier());
   result->AppendMatches(input, matches);
+  result->set_headers_map({{7, base::UTF8ToUTF16("header")}});
   result->SortAndCull(input, nullptr);
   popup_model()->OnResultChanged();
   EXPECT_EQ(0u, model()->popup_model()->selected_line());
@@ -498,6 +501,7 @@ TEST_F(OmniboxPopupModelSuggestionButtonRowTest,
                           metrics::OmniboxEventProto::NTP,
                           TestSchemeClassifier());
   result->AppendMatches(input, matches);
+  result->set_headers_map({{7, base::UTF8ToUTF16("header")}});
   result->SortAndCull(input, nullptr);
   popup_model()->OnResultChanged();
   EXPECT_EQ(0u, model()->popup_model()->selected_line());
@@ -591,6 +595,7 @@ TEST_F(OmniboxPopupModelTest, PopupInlineAutocompleteAndTemporaryText) {
                           metrics::OmniboxEventProto::NTP,
                           TestSchemeClassifier());
   result->AppendMatches(input, matches);
+  result->set_headers_map({{7, base::UTF8ToUTF16("header")}});
   result->SortAndCull(input, nullptr);
   popup_model()->OnResultChanged();