[omnibox] Merge new header info with existing one rather than replacing it

- Merges the new header info with the old one in ACResult rather than
  replacing it. The old header info may be needed by the old matches.
- Clears the header info along with matches in AutocompleteResult::Reset
- Moves the logic to strip the match group IDs that don't have an
  equivalent header string next to the logic for grouping and demoting
  matches with headers in ACResult for improved readability.  

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

chrome/browser/android/omnibox/autocomplete_controller_android.cc

@@ -662,14 +662,12 @@ void AutocompleteControllerAndroid::PopulateOmniboxGroupsDetails(
     JNIEnv* env,
     ScopedJavaLocalRef<jobject> j_autocomplete_result,
     const SearchSuggestionParser::HeadersMap& native_header_map,
-    const std::vector<int>& hidden_group_ids) {
-  base::flat_set<int> hidden_group_ids_set = hidden_group_ids;
-
+    const std::set<int>& hidden_group_ids) {
   for (const auto& group_header : native_header_map) {
     Java_AutocompleteController_addOmniboxGroupDetailsToResult(
         env, j_autocomplete_result, group_header.first,
         ConvertUTF16ToJavaString(env, group_header.second),
-        hidden_group_ids_set.contains(group_header.first));
+        base::Contains(hidden_group_ids, group_header.first));
   }
 }
 

chrome/browser/android/omnibox/autocomplete_controller_android.h

@@ -145,7 +145,7 @@ class AutocompleteControllerAndroid : public AutocompleteController::Observer,
       JNIEnv* env,
       base::android::ScopedJavaLocalRef<jobject> j_autocomplete_result,
       const SearchSuggestionParser::HeadersMap& header_map,
-      const std::vector<int>& hidden_group_ids);
+      const std::set<int>& hidden_group_ids);
 
   // A helper method for fetching the top synchronous autocomplete result.
   // The |prevent_inline_autocomplete| flag is passed to the AutocompleteInput

components/omnibox/browser/autocomplete_controller.cc

@@ -811,27 +811,11 @@ void AutocompleteController::UpdateHeaderInfoFromZeroSuggestProvider(
   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.
-      // 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);
-
-      const base::string16 header = result->GetHeaderForGroupId(group_id);
-      if (!header.empty()) {
-        match.RecordAdditionalInfo("header string", header);
-      } else {
-        // Strip all match group IDs that don't have a header string. Otherwise,
-        // these matches will be shown at the bottom with an empty header row.
-        // They should be treated as an ordinary match with no group ID.
-        match.suggestion_group_id.reset();
-      }
-    }
-  }
+  // Merge the new header info with the existing one rather than replacing it.
+  // We might end up using the existing matches fully or partially if there are
+  // not enough new ones. Thus, we should also keep the existing header info.
+  result->MergeHeadersMap(zero_suggest_provider_->headers_map());
+  result->MergeHiddenGroupIds(zero_suggest_provider_->hidden_group_ids());
 }
 
 void AutocompleteController::UpdateKeywordDescriptions(

components/omnibox/browser/autocomplete_controller.h

@@ -229,9 +229,7 @@ class AutocompleteController : public AutocompleteProviderListener,
   void UpdateAssociatedKeywords(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.
+  // well as the set of hidden suggestion group IDs.
   // 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.

components/omnibox/browser/autocomplete_result.cc

@@ -332,10 +332,20 @@ void AutocompleteResult::GroupAndDemoteMatchesWithHeaders() {
   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());
+      // Record group IDs and header strings, if available, into the
+      // additional_info field for chrome://omnibox.
+      int group_id = it->suggestion_group_id.value();
+      it->RecordAdditionalInfo("suggestion_group_id", group_id);
+      const base::string16 header = GetHeaderForGroupId(group_id);
+      if (!header.empty()) {
+        it->RecordAdditionalInfo("header string", header);
+      } else {
+        // Strip group IDs for which there is no header string from the matches.
+        // Otherwise, these matches may be shown at the bottom with an empty
+        // header row. They should instead be treated as ordinary matches with
+        // no group ID.
+        it->suggestion_group_id.reset();
+      }
     }
 
     int group_id =
@@ -665,6 +675,8 @@ size_t AutocompleteResult::CalculateNumMatchesPerUrlCount(
 
 void AutocompleteResult::Reset() {
   matches_.clear();
+  headers_map_.clear();
+  hidden_group_ids_.clear();
 }
 
 void AutocompleteResult::Swap(AutocompleteResult* other) {
@@ -813,6 +825,16 @@ bool AutocompleteResult::IsSuggestionGroupIdHidden(
   return base::Contains(hidden_group_ids_, suggestion_group_id);
 }
 
+void AutocompleteResult::MergeHeadersMap(
+    const SearchSuggestionParser::HeadersMap& headers_map) {
+  headers_map_.insert(headers_map.begin(), headers_map.end());
+}
+
+void AutocompleteResult::MergeHiddenGroupIds(
+    const std::vector<int>& hidden_group_ids) {
+  hidden_group_ids_.insert(hidden_group_ids.begin(), hidden_group_ids.end());
+}
+
 // static
 void AutocompleteResult::LogAsynchronousUpdateMetrics(
     const std::vector<MatchDedupComparator>& old_result,

components/omnibox/browser/autocomplete_result.h

@@ -84,6 +84,10 @@ class AutocompleteResult {
   // 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.
+  // Note that prior to grouping and demoting the matches with headers, we strip
+  // all match group IDs that don't have an equivalent header string;
+  // essentially treating those matches as if they did not belong to any
+  // suggestion group.
   // Called after matches are deduped and sorted and before they are culled.
   void GroupAndDemoteMatchesWithHeaders();
 
@@ -150,7 +154,7 @@ class AutocompleteResult {
     return headers_map_;
   }
 
-  const std::vector<int>& hidden_group_ids() const { return hidden_group_ids_; }
+  const std::set<int>& hidden_group_ids() const { return hidden_group_ids_; }
 
   // Clears the matches for this result set.
   void Reset();
@@ -191,20 +195,16 @@ class AutocompleteResult {
   bool IsSuggestionGroupIdHidden(PrefService* prefs,
                                  int suggestion_group_id) const;
 
+  void MergeHeadersMap(const SearchSuggestionParser::HeadersMap& headers_map);
+
+  void MergeHiddenGroupIds(const std::vector<int>& hidden_group_ids);
+
   // Logs metrics for when |new_result| replaces |old_result| asynchronously.
   // |old_result| a list of the comparators for the old matches.
   static void LogAsynchronousUpdateMetrics(
       const std::vector<MatchDedupComparator>& old_result,
       const AutocompleteResult& new_result);
 
-  void set_headers_map(const SearchSuggestionParser::HeadersMap& headers_map) {
-    headers_map_ = headers_map;
-  }
-
-  void set_hidden_group_ids(const std::vector<int>& hidden_group_ids) {
-    hidden_group_ids_ = hidden_group_ids;
-  }
-
   // This value should be comfortably larger than any max-autocomplete-matches
   // under consideration.
   static constexpr size_t kMaxAutocompletePositionValue = 30;
@@ -306,8 +306,7 @@ class AutocompleteResult {
   SearchSuggestionParser::HeadersMap headers_map_;
 
   // The server supplied list of group IDs that should be hidden-by-default.
-  // Typical size is 0 to 3, from one provider. That's why it's not a set.
-  std::vector<int> hidden_group_ids_;
+  std::set<int> hidden_group_ids_;
 };
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_RESULT_H_

components/omnibox/browser/omnibox_popup_model_unittest.cc

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