[omnibox] Persist to pref when a user manually shows a suggestion group

Currently, we persist to a pref when a user manually hides a suggestion
group. This works because all suggestion groups are default-shown.

In the near future, we plan to have some suggestion groups be
default-hidden. So now, we should persist if a user has manually shown
a suggestion group as well.

(See go/desktop-chrome-polaris-engdesign for details.)

This CL obsoletes the "omnibox.hiddenGroupIds" list pref and instead
creates a new "omnibox.suggestionGroupVisibility" dictionary pref,
which has the group ID as the key, and stores two possible states:
 - HIDDEN (1)
 - SHOWN (2)

If the user has never set the preference, we plan to eventually return
a third value:
 - DEFAULT (0)

In that case, the callsite will have to decide on the proper default
visibility, which will be either a client-side flag or a
server-provided hint per-group.

One more thing:

I've purposely not provided a migration path from the old pref to the
new pref. I did this for simplicity sake. I've looked at the UMA, and
there are only ~25 count usages of the collapse / expand feature each
on Stable in the last 30 days.

Across all Channels, there's ~550 each over the last 30 days. I think
a significant fraction of that 550 is Chromium developers.

In other words, I think simplicity trumps migrating the prefs of the
scant few users who've set this pref so far.

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

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

@@ -157,9 +157,10 @@ OmniboxPopupContentsView::OmniboxPopupContentsView(
     pref_change_registrar_.Init(pref_service);
     // Unretained is appropriate here. 'this' will outlive the registrar.
     pref_change_registrar_.Add(
-        omnibox::kOmniboxHiddenGroupIds,
-        base::BindRepeating(&OmniboxPopupContentsView::OnHiddenGroupIdsUpdate,
-                            base::Unretained(this)));
+        omnibox::kSuggestionGroupVisibility,
+        base::BindRepeating(
+            &OmniboxPopupContentsView::OnSuggestionGroupVisibilityUpdate,
+            base::Unretained(this)));
   }
 }
 
@@ -532,7 +533,7 @@ size_t OmniboxPopupContentsView::GetIndexForPoint(const gfx::Point& point) {
   return OmniboxPopupModel::kNoMatch;
 }
 
-void OmniboxPopupContentsView::OnHiddenGroupIdsUpdate() {
+void OmniboxPopupContentsView::OnSuggestionGroupVisibilityUpdate() {
   for (size_t i = 0; i < model_->result().size(); ++i) {
     const AutocompleteMatch& match = model_->result().match_at(i);
     bool match_hidden =

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

@@ -107,8 +107,8 @@ class OmniboxPopupContentsView : public views::View,
   // the specified point.
   size_t GetIndexForPoint(const gfx::Point& point);
 
-  // Update which result views are visible when the hidden group IDs change.
-  void OnHiddenGroupIdsUpdate();
+  // Update which result views are visible when the group visibility changes.
+  void OnSuggestionGroupVisibilityUpdate();
 
   // Gets the pref service for this view. May return nullptr in tests.
   PrefService* GetPrefService() const;

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

@@ -66,7 +66,7 @@ class OmniboxRowView::HeaderView : public views::View,
     if (row_view_->pref_service_) {
       pref_change_registrar_.Init(row_view_->pref_service_);
       // Unretained is appropriate here. 'this' will outlive the registrar.
-      pref_change_registrar_.Add(omnibox::kOmniboxHiddenGroupIds,
+      pref_change_registrar_.Add(omnibox::kSuggestionGroupVisibility,
                                  base::BindRepeating(&HeaderView::OnPrefChanged,
                                                      base::Unretained(this)));
     }

components/omnibox/browser/autocomplete_match.h

@@ -553,6 +553,9 @@ struct AutocompleteMatch {
   // The optional suggestion group Id based on the SuggestionGroupIds enum in
   // suggestion_config.proto. Used to look up the header text this match must
   // appear under from ACResult.
+  //
+  // If this value exists, it should always be positive and nonzero. In Java and
+  // JavaScript, -1 is used as a sentinel value, but should never occur in C++.
   base::Optional<int> suggestion_group_id;
 
   // If true, UI-level code should swap the contents and description fields

components/omnibox/browser/omnibox_prefs.cc

@@ -6,7 +6,9 @@
 
 #include "base/check.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/optional.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ -23,9 +25,10 @@ const char kToggleSuggestionGroupIdOnHistogram[] =
 // Also gated by a feature and server-side Admin Panel controls.
 const char kDocumentSuggestEnabled[] = "documentsuggest.enabled";
 
-// A list of suggestion group IDs for zero suggest that are not allowed to
-// appear in the results.
-const char kOmniboxHiddenGroupIds[] = "omnibox.hiddenGroupIds";
+// A dictionary of visibility preferences for suggestion groups. The key is the
+// suggestion group ID serialized as a string, and the value is
+// SuggestionGroupVisibility serialized as an integer.
+const char kSuggestionGroupVisibility[] = "omnibox.suggestionGroupVisibility";
 
 // Boolean that specifies whether to always show full URLs in the omnibox.
 const char kPreventUrlElisionsInOmnibox[] = "omnibox.prevent_url_elisions";
@@ -34,33 +37,62 @@ const char kPreventUrlElisionsInOmnibox[] = "omnibox.prevent_url_elisions";
 const char kZeroSuggestCachedResults[] = "zerosuggest.cachedresults";
 
 void RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterListPref(omnibox::kOmniboxHiddenGroupIds,
-                             base::Value(base::Value::Type::LIST));
+  registry->RegisterDictionaryPref(kSuggestionGroupVisibility);
 }
 
-bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id) {
+SuggestionGroupVisibility GetSuggestionGroupVisibility(
+    PrefService* prefs,
+    int suggestion_group_id) {
   DCHECK(prefs);
-  const base::ListValue* group_id_values =
-      prefs->GetList(kOmniboxHiddenGroupIds);
-  return std::find(group_id_values->begin(), group_id_values->end(),
-                   base::Value(suggestion_group_id)) != group_id_values->end();
+
+  const base::DictionaryValue* dictionary =
+      prefs->GetDictionary(kSuggestionGroupVisibility);
+  DCHECK(dictionary);
+
+  base::Optional<int> value =
+      dictionary->FindIntKey(base::NumberToString(suggestion_group_id));
+
+  if (value == SuggestionGroupVisibility::HIDDEN ||
+      value == SuggestionGroupVisibility::SHOWN) {
+    return static_cast<SuggestionGroupVisibility>(*value);
+  }
+
+  return SuggestionGroupVisibility::DEFAULT;
 }
 
-void ToggleSuggestionGroupIdVisibility(PrefService* prefs,
-                                       int suggestion_group_id) {
+bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id) {
+  // TODO(tommycli): For now, this preserves the legacy behavior of DEFAULT
+  // meaning SHOWN. Next, callsites need to have some idea of whether a group
+  // should be shown or hidden by default, likely provided by a server hint.
+  return GetSuggestionGroupVisibility(prefs, suggestion_group_id) ==
+         SuggestionGroupVisibility::HIDDEN;
+}
+
+void SetSuggestionGroupVisibility(PrefService* prefs,
+                                  int suggestion_group_id,
+                                  SuggestionGroupVisibility new_value) {
   DCHECK(prefs);
-  ListPrefUpdate update(prefs, kOmniboxHiddenGroupIds);
-  const bool is_hidden = IsSuggestionGroupIdHidden(prefs, suggestion_group_id);
-  if (is_hidden) {
-    update->EraseListValue(base::Value(suggestion_group_id));
-  } else {
-    update->Append(suggestion_group_id);
-  }
+
+  DictionaryPrefUpdate update(prefs, kSuggestionGroupVisibility);
+  update->SetIntKey(base::NumberToString(suggestion_group_id), new_value);
+
   base::SparseHistogram::FactoryGet(
-      is_hidden ? kToggleSuggestionGroupIdOnHistogram
-                : kToggleSuggestionGroupIdOffHistogram,
+      new_value == SuggestionGroupVisibility::SHOWN
+          ? kToggleSuggestionGroupIdOnHistogram
+          : kToggleSuggestionGroupIdOffHistogram,
       base::HistogramBase::kUmaTargetedHistogramFlag)
       ->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

@@ -12,6 +12,21 @@ class PrefService;
 
 namespace omnibox {
 
+// These values are persisted to prefs. They cannot be freely changed.
+enum SuggestionGroupVisibility {
+  // When DEFAULT is returned, the group's visibility should be controlled by
+  // the server-provided hint.
+  DEFAULT = 0,
+
+  // HIDDEN means the user has manually hidden the group before, and so this
+  // group should be hidden regardless of the server-provided hint.
+  HIDDEN = 1,
+
+  // SHOWN means the user has manually shown the group before, and so this
+  // group should be shown regardless of the server-provided hint.
+  SHOWN = 2,
+};
+
 // Histograms being recorded when visibility of suggestion group IDs change.
 extern const char kToggleSuggestionGroupIdOffHistogram[];
 extern const char kToggleSuggestionGroupIdOnHistogram[];
@@ -19,7 +34,7 @@ extern const char kToggleSuggestionGroupIdOnHistogram[];
 // Alphabetical list of preference names specific to the omnibox component.
 // Keep alphabetized, and document each in the .cc file.
 extern const char kDocumentSuggestEnabled[];
-extern const char kOmniboxHiddenGroupIds[];
+extern const char kSuggestionGroupVisibility[];
 extern const char kPreventUrlElisionsInOmnibox[];
 extern const char kZeroSuggestCachedResults[];
 
@@ -29,9 +44,16 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry);
 // results.
 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);