[omnibox] Update UI to use AutocompleteResult::IsSuggestionGroupIdHidden

This CL updates all the UI callsites to use the new
AutocompleteResult::IsSuggestionGroupIdHidden() method, which respects
the server-provided hint as to which groups should default-hidden.

This CL also deletes the deprecated omnibox::IsSuggestionGroupIdHidden()
method, which is no longer used after this CL.

This CL also deletes the deprecated
omnibox::ToggleSuggestionGroupIdVisibility() method, and migrates all
callsites to the omnibox::SetSuggestionGroupVisbility() method.

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

chrome/browser/ui/search/omnibox_mojo_utils.cc

@@ -22,6 +22,7 @@ namespace {
 
 base::flat_map<int32_t, search::mojom::SuggestionGroupPtr>
 CreateSuggestionGroupsMap(
+    const AutocompleteResult& result,
     PrefService* prefs,
     const SearchSuggestionParser::HeadersMap& headers_map) {
   base::flat_map<int32_t, search::mojom::SuggestionGroupPtr> result_map;
@@ -30,7 +31,7 @@ CreateSuggestionGroupsMap(
         search::mojom::SuggestionGroup::New();
     suggestion_group->header = pair.second;
     suggestion_group->hidden =
-        omnibox::IsSuggestionGroupIdHidden(prefs, pair.first);
+        result.IsSuggestionGroupIdHidden(prefs, pair.first);
     result_map.emplace(pair.first, std::move(suggestion_group));
   }
   return result_map;
@@ -143,7 +144,7 @@ search::mojom::AutocompleteResultPtr CreateAutocompleteResult(
     const AutocompleteResult& result,
     PrefService* prefs) {
   return search::mojom::AutocompleteResult::New(
-      input, CreateSuggestionGroupsMap(prefs, result.headers_map()),
+      input, CreateSuggestionGroupsMap(result, prefs, result.headers_map()),
       CreateAutocompleteMatches(result));
 }
 

chrome/browser/ui/search/search_tab_helper.cc

@@ -752,8 +752,13 @@ void SearchTabHelper::ToggleSuggestionGroupIdVisibility(
   if (!autocomplete_controller_)
     return;
 
-  omnibox::ToggleSuggestionGroupIdVisibility(profile()->GetPrefs(),
-                                             suggestion_group_id);
+  omnibox::SuggestionGroupVisibility new_value =
+      autocomplete_controller_->result().IsSuggestionGroupIdHidden(
+          profile()->GetPrefs(), suggestion_group_id)
+          ? omnibox::SuggestionGroupVisibility::SHOWN
+          : omnibox::SuggestionGroupVisibility::HIDDEN;
+  omnibox::SetSuggestionGroupVisibility(profile()->GetPrefs(),
+                                        suggestion_group_id, new_value);
 }
 
 void SearchTabHelper::LogCharTypedToRepaintLatency(uint32_t latency_ms) {

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc

@@ -367,7 +367,7 @@ void OmniboxPopupContentsView::UpdatePopupAppearance() {
       // Set visibility of the result view based on whether the group is hidden.
       bool match_hidden = pref_service &&
                           match.suggestion_group_id.has_value() &&
-                          omnibox::IsSuggestionGroupIdHidden(
+                          model_->result().IsSuggestionGroupIdHidden(
                               pref_service, match.suggestion_group_id.value());
       result_view->SetVisible(!match_hidden);
 
@@ -556,8 +556,8 @@ void OmniboxPopupContentsView::OnSuggestionGroupVisibilityUpdate() {
     const AutocompleteMatch& match = model_->result().match_at(i);
     bool match_hidden =
         match.suggestion_group_id.has_value() &&
-        omnibox::IsSuggestionGroupIdHidden(GetPrefService(),
-                                           match.suggestion_group_id.value());
+        model_->result().IsSuggestionGroupIdHidden(
+            GetPrefService(), match.suggestion_group_id.value());
     if (OmniboxResultView* result_view = result_view_at(i))
       result_view->SetVisible(!match_hidden);
   }

chrome/browser/ui/views/omnibox/omnibox_row_view.cc

@@ -82,8 +82,9 @@ class OmniboxRowView::HeaderView : public views::View,
     header_label_->SetText(base::i18n::ToUpper(header_text_));
 
     if (row_view_->pref_service_) {
-      suggestion_group_hidden_ = omnibox::IsSuggestionGroupIdHidden(
-          row_view_->pref_service_, suggestion_group_id_);
+      suggestion_group_hidden_ =
+          row_view_->popup_model_->result().IsSuggestionGroupIdHidden(
+              row_view_->pref_service_, suggestion_group_id_);
 
       header_toggle_button_->SetToggled(suggestion_group_hidden_);
     }
@@ -192,8 +193,9 @@ class OmniboxRowView::HeaderView : public views::View,
   void OnPrefChanged() {
     DCHECK(row_view_->pref_service_);
     bool was_hidden = suggestion_group_hidden_;
-    suggestion_group_hidden_ = omnibox::IsSuggestionGroupIdHidden(
-        row_view_->pref_service_, suggestion_group_id_);
+    suggestion_group_hidden_ =
+        row_view_->popup_model_->result().IsSuggestionGroupIdHidden(
+            row_view_->pref_service_, suggestion_group_id_);
 
     if (was_hidden != suggestion_group_hidden_) {
       NotifyAccessibilityEvent(ax::mojom::Event::kExpandedChanged, true);

chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc

@@ -1014,8 +1014,16 @@ void NewTabPageHandler::DeleteAutocompleteMatch(uint8_t line) {
 
 void NewTabPageHandler::ToggleSuggestionGroupIdVisibility(
     int32_t suggestion_group_id) {
-  omnibox::ToggleSuggestionGroupIdVisibility(profile_->GetPrefs(),
-                                             suggestion_group_id);
+  if (!autocomplete_controller_)
+    return;
+
+  omnibox::SuggestionGroupVisibility new_value =
+      autocomplete_controller_->result().IsSuggestionGroupIdHidden(
+          profile_->GetPrefs(), suggestion_group_id)
+          ? omnibox::SuggestionGroupVisibility::SHOWN
+          : omnibox::SuggestionGroupVisibility::HIDDEN;
+  omnibox::SetSuggestionGroupVisibility(profile_->GetPrefs(),
+                                        suggestion_group_id, new_value);
 }
 
 void NewTabPageHandler::LogCharTypedToRepaintLatency(base::TimeDelta latency) {

components/omnibox/browser/autocomplete_result.cc

@@ -755,8 +755,9 @@ base::string16 AutocompleteResult::GetHeaderForGroupId(
   return base::string16();
 }
 
-bool AutocompleteResult::IsSuggestionGroupIdHidden(PrefService* prefs,
-                                                   int suggestion_group_id) {
+bool AutocompleteResult::IsSuggestionGroupIdHidden(
+    PrefService* prefs,
+    int suggestion_group_id) const {
   omnibox::SuggestionGroupVisibility user_preference =
       omnibox::GetUserPreferenceForSuggestionGroupVisibility(
           prefs, suggestion_group_id);

components/omnibox/browser/autocomplete_result.h

@@ -171,7 +171,8 @@ class AutocompleteResult {
   // Returns whether or not |suggestion_group_id| should be collapsed in the UI.
   // This method takes into account both the user's stored |prefs| as well as
   // the server-provided visibility hint for |suggestion_group_id|.
-  bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id);
+  bool IsSuggestionGroupIdHidden(PrefService* prefs,
+                                 int suggestion_group_id) const;
 
   // Logs metrics for when |new_result| replaces |old_result| asynchronously.
   // |old_result| a list of the comparators for the old matches.

components/omnibox/browser/omnibox_popup_model.cc

@@ -477,7 +477,7 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const {
   // user is trying to focus the header itself (which is still shown).
   if (selection.state != FOCUSED_BUTTON_HEADER &&
       match.suggestion_group_id.has_value() && pref_service_ &&
-      omnibox::IsSuggestionGroupIdHidden(pref_service_,
+      result().IsSuggestionGroupIdHidden(pref_service_,
                                          match.suggestion_group_id.value())) {
     return false;
   }
@@ -534,12 +534,18 @@ bool OmniboxPopupModel::TriggerSelectionAction(Selection selection,
 
   auto& match = result().match_at(selection.line);
   switch (selection.state) {
-    case FOCUSED_BUTTON_HEADER:
+    case FOCUSED_BUTTON_HEADER: {
       DCHECK(match.suggestion_group_id.has_value());
-      omnibox::ToggleSuggestionGroupIdVisibility(
-          pref_service_, match.suggestion_group_id.value());
-      break;
 
+      omnibox::SuggestionGroupVisibility new_value =
+          result().IsSuggestionGroupIdHidden(pref_service_,
+                                             match.suggestion_group_id.value())
+              ? omnibox::SuggestionGroupVisibility::SHOWN
+              : omnibox::SuggestionGroupVisibility::HIDDEN;
+      omnibox::SetSuggestionGroupVisibility(
+          pref_service_, match.suggestion_group_id.value(), new_value);
+      break;
+    }
     case FOCUSED_BUTTON_KEYWORD:
       // TODO(yoangela): Merge logic with mouse/gesture events in
       // OmniboxSuggestionButtonRowView::ButtonPressed - This case currently
@@ -593,7 +599,7 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(
   int additional_message_id = 0;
   switch (selection_.state) {
     case FOCUSED_BUTTON_HEADER: {
-      bool group_hidden = omnibox::IsSuggestionGroupIdHidden(
+      bool group_hidden = result().IsSuggestionGroupIdHidden(
           pref_service_, match.suggestion_group_id.value());
       int message_id = group_hidden ? IDS_ACC_HEADER_SHOW_SUGGESTIONS_BUTTON
                                     : IDS_ACC_HEADER_HIDE_SUGGESTIONS_BUTTON;

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -285,7 +285,8 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelectionWithHiddenGroupIds) {
   // Hide the second two matches.
   matches[2].suggestion_group_id = 7;
   matches[3].suggestion_group_id = 7;
-  omnibox::ToggleSuggestionGroupIdVisibility(pref_service(), 7);
+  omnibox::SetSuggestionGroupVisibility(
+      pref_service(), 7, omnibox::SuggestionGroupVisibility::HIDDEN);
 
   auto* result = &model()->autocomplete_controller()->result_;
   AutocompleteInput input(base::UTF8ToUTF16("match"),

components/omnibox/browser/omnibox_prefs.cc

@@ -60,13 +60,6 @@ SuggestionGroupVisibility GetUserPreferenceForSuggestionGroupVisibility(
   return SuggestionGroupVisibility::DEFAULT;
 }
 
-bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id) {
-  // TODO(tommycli): Migrate all callsites to
-  // AutocompleteResult::IsSuggestionGroupIdHidden().
-  return GetUserPreferenceForSuggestionGroupVisibility(
-             prefs, suggestion_group_id) == SuggestionGroupVisibility::HIDDEN;
-}
-
 void SetSuggestionGroupVisibility(PrefService* prefs,
                                   int suggestion_group_id,
                                   SuggestionGroupVisibility new_value) {
@@ -83,15 +76,4 @@ void SetSuggestionGroupVisibility(PrefService* prefs,
       ->Add(suggestion_group_id);
 }
 
-void ToggleSuggestionGroupIdVisibility(PrefService* prefs,
-                                       int suggestion_group_id) {
-  // TODO(tommycli): Migrate all callsites to use SetSuggestionGroupVisibility()
-  // instead of this method.
-  SuggestionGroupVisibility new_value =
-      IsSuggestionGroupIdHidden(prefs, suggestion_group_id)
-          ? SuggestionGroupVisibility::SHOWN
-          : SuggestionGroupVisibility::HIDDEN;
-  SetSuggestionGroupVisibility(prefs, suggestion_group_id, new_value);
-}
-
 }  // namespace omnibox

components/omnibox/browser/omnibox_prefs.h

@@ -46,33 +46,16 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry);
 //
 // Warning: UI code should use AutocompleteResult::IsSuggestionGroupIdHidden()
 // instead, which uses the server-provided hint on default-hidden groups.
-//
-// TODO(tommycli): Once all UI code has migrated to calling
-// AutocompleteResult::IsSuggestionGroupIdHidden(), we can likely make this
-// method an inaccessible anonymous function.
+// This method is accessible for testing only.
 SuggestionGroupVisibility GetUserPreferenceForSuggestionGroupVisibility(
     PrefService* prefs,
     int suggestion_group_id);
 
-// Returns whether the given suggestion group ID is allowed to appear in the
-// results.
-//
-// DEPRECATED: Use AutocompleteResult::IsSuggestionGroupIdHidden() instead.
-bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id);
-
 // Sets the group visibility of |suggestion_group_id| to |new_value|.
 void SetSuggestionGroupVisibility(PrefService* prefs,
                                   int suggestion_group_id,
                                   SuggestionGroupVisibility new_value);
 
-// Allows suggestions with the given suggestion group ID to appear in the
-// results if they currently are not allowed to or prevents them from
-// appearing in the results if they are currently permitted to.
-//
-// DEPRECATED: Use SetSuggestionGroupVisibility() instead.
-void ToggleSuggestionGroupIdVisibility(PrefService* prefs,
-                                       int suggestion_group_id);
-
 }  // namespace omnibox
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_PREFS_H_

components/omnibox/browser/omnibox_prefs_unittest.cc

@@ -7,10 +7,7 @@
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using omnibox::IsSuggestionGroupIdHidden;
-using omnibox::kToggleSuggestionGroupIdOffHistogram;
-using omnibox::kToggleSuggestionGroupIdOnHistogram;
-using omnibox::ToggleSuggestionGroupIdVisibility;
+namespace omnibox {
 
 class OmniboxPrefsTest : public ::testing::Test {
  public:
@@ -35,41 +32,58 @@ TEST_F(OmniboxPrefsTest, SuggestionGroupId) {
   const int kOnboardingGroupId = 40001;
   const int kRZPSGroupId = 40009;
   {
-    // Expect |kOnboardingGroupId| to be visible.
-    EXPECT_FALSE(IsSuggestionGroupIdHidden(GetPrefs(), kOnboardingGroupId));
+    // Expect |kOnboardingGroupId| to be in the default state.
+    EXPECT_EQ(SuggestionGroupVisibility::DEFAULT,
+              GetUserPreferenceForSuggestionGroupVisibility(
+                  GetPrefs(), kOnboardingGroupId));
     histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOffHistogram, 0);
 
-    // Expect |kRZPSGroupId| to be visible.
-    EXPECT_FALSE(IsSuggestionGroupIdHidden(GetPrefs(), kRZPSGroupId));
+    // Expect |kRZPSGroupId| to be in the default state.
+    EXPECT_EQ(SuggestionGroupVisibility::DEFAULT,
+              GetUserPreferenceForSuggestionGroupVisibility(GetPrefs(),
+                                                            kRZPSGroupId));
     histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOnHistogram, 0);
   }
   {
-    ToggleSuggestionGroupIdVisibility(GetPrefs(), kOnboardingGroupId);
+    SetSuggestionGroupVisibility(GetPrefs(), kOnboardingGroupId,
+                                 SuggestionGroupVisibility::HIDDEN);
 
     // Expect |kOnboardingGroupId| to have been toggled hidden.
-    EXPECT_TRUE(IsSuggestionGroupIdHidden(GetPrefs(), kOnboardingGroupId));
+    EXPECT_EQ(SuggestionGroupVisibility::HIDDEN,
+              GetUserPreferenceForSuggestionGroupVisibility(
+                  GetPrefs(), kOnboardingGroupId));
     histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOffHistogram, 1);
     histogram()->ExpectBucketCount(kToggleSuggestionGroupIdOffHistogram,
                                    kOnboardingGroupId, 1);
 
-    // Expect |kRZPSGroupId| to have remained visible.
-    EXPECT_FALSE(IsSuggestionGroupIdHidden(GetPrefs(), kRZPSGroupId));
+    // Expect |kRZPSGroupId| to have remained in the default state.
+    EXPECT_EQ(SuggestionGroupVisibility::DEFAULT,
+              GetUserPreferenceForSuggestionGroupVisibility(GetPrefs(),
+                                                            kRZPSGroupId));
     histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOnHistogram, 0);
   }
   {
-    ToggleSuggestionGroupIdVisibility(GetPrefs(), kOnboardingGroupId);
-    ToggleSuggestionGroupIdVisibility(GetPrefs(), kRZPSGroupId);
-
-    // Expect |kRZPSGroupId| to have been toggled hidden.
-    EXPECT_TRUE(IsSuggestionGroupIdHidden(GetPrefs(), kRZPSGroupId));
-    histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOffHistogram, 2);
-    histogram()->ExpectBucketCount(kToggleSuggestionGroupIdOffHistogram,
-                                   kRZPSGroupId, 1);
+    SetSuggestionGroupVisibility(GetPrefs(), kOnboardingGroupId,
+                                 SuggestionGroupVisibility::SHOWN);
+    SetSuggestionGroupVisibility(GetPrefs(), kRZPSGroupId,
+                                 SuggestionGroupVisibility::HIDDEN);
 
     // Expect |kOnboardingGroupId| to have been toggled visible again.
-    EXPECT_FALSE(IsSuggestionGroupIdHidden(GetPrefs(), kOnboardingGroupId));
+    EXPECT_EQ(SuggestionGroupVisibility::SHOWN,
+              GetUserPreferenceForSuggestionGroupVisibility(
+                  GetPrefs(), kOnboardingGroupId));
     histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOnHistogram, 1);
     histogram()->ExpectBucketCount(kToggleSuggestionGroupIdOnHistogram,
                                    kOnboardingGroupId, 1);
+
+    // Expect |kRZPSGroupId| to have been toggled hidden.
+    EXPECT_EQ(SuggestionGroupVisibility::HIDDEN,
+              GetUserPreferenceForSuggestionGroupVisibility(GetPrefs(),
+                                                            kRZPSGroupId));
+    histogram()->ExpectTotalCount(kToggleSuggestionGroupIdOffHistogram, 2);
+    histogram()->ExpectBucketCount(kToggleSuggestionGroupIdOffHistogram,
+                                   kRZPSGroupId, 1);
   }
 }
+
+}  // namespace omnibox