[Extensions] Refactor profile wide ExtensionPref with basic types.

This is the first stage in a larger refactor, which adds support for
better typing in prefs while allowing extension prefs to remain agnostic.
This CL implements the getter and setter methods for profile wide
ExtensionPrefs with string, integer and boolean values.

Design Doc: http://go/extprefrefactor

Bug: 1069560
Change-Id: I6b8901bd1856d9afbadf358229377fc2db665cfc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2276497
Commit-Queue: Archana Simha <archanasimha@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789139}

chrome/browser/extensions/chrome_content_verifier_delegate.cc

@@ -31,6 +31,7 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/management_policy.h"
+#include "extensions/browser/pref_types.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/extensions_client.h"
 #include "extensions/common/manifest.h"
@@ -267,7 +268,8 @@ void ChromeContentVerifierDelegate::VerifyFailed(
                 << "might want to use a ScopedIgnoreContentVerifierForTest "
                 << "instance to prevent this.";
   service->DisableExtension(extension_id, disable_reason::DISABLE_CORRUPTED);
-  ExtensionPrefs::Get(context_)->IncrementCorruptedDisableCount();
+  ExtensionPrefs::Get(context_)->IncrementPref(
+      extensions::kCorruptedDisableCount);
   UMA_HISTOGRAM_BOOLEAN("Extensions.CorruptExtensionBecameDisabled", true);
   UMA_HISTOGRAM_ENUMERATION("Extensions.CorruptExtensionDisabledReason", reason,
                             ContentVerifyJob::FAILURE_REASON_MAX);

chrome/browser/extensions/extension_prefs_unittest.cc

@@ -28,6 +28,7 @@
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/install_flag.h"
 #include "extensions/browser/pref_names.h"
+#include "extensions/browser/pref_types.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/extension_id.h"
@@ -1332,4 +1333,30 @@ TEST_F(ExtensionPrefsSimpleTest, MigrateToNewExternalUninstallBits) {
       prefs.prefs()->IsExternalExtensionUninstalled(internal_extension));
 }
 
+// Tests the generic Get/Set functions for profile wide extension prefs.
+TEST_F(ExtensionPrefsSimpleTest, ProfileExtensionPrefsMapTest) {
+  constexpr PrefMap kTestBooleanPref = {"test.boolean", PrefType::kBool,
+                                        PrefScope::kProfile};
+  constexpr PrefMap kTestIntegerPref = {"test.integer", PrefType::kInteger,
+                                        PrefScope::kProfile};
+  constexpr PrefMap kTestStringPref = {"test.string", PrefType::kString,
+                                       PrefScope::kProfile};
+
+  content::BrowserTaskEnvironment task_environment_;
+  TestExtensionPrefs prefs(base::ThreadTaskRunnerHandle::Get());
+
+  auto* registry = prefs.pref_registry().get();
+  registry->RegisterBooleanPref(kTestBooleanPref.name, false);
+  registry->RegisterIntegerPref(kTestIntegerPref.name, 0);
+  registry->RegisterStringPref(kTestStringPref.name, std::string());
+
+  prefs.prefs()->SetBooleanPref(kTestBooleanPref, true);
+  prefs.prefs()->SetIntegerPref(kTestIntegerPref, 1);
+  prefs.prefs()->SetStringPref(kTestStringPref, "foo");
+
+  EXPECT_TRUE(prefs.prefs()->GetPrefAsBoolean(kTestBooleanPref));
+  EXPECT_EQ(prefs.prefs()->GetPrefAsInteger(kTestIntegerPref), 1);
+  EXPECT_EQ(prefs.prefs()->GetPrefAsString(kTestStringPref), "foo");
+}
+
 }  // namespace extensions

chrome/browser/extensions/installed_loader.cc

@@ -34,6 +34,7 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
 #include "extensions/browser/management_policy.h"
+#include "extensions/browser/pref_types.h"
 #include "extensions/browser/ui_util.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_l10n_util.h"
@@ -671,8 +672,9 @@ void InstalledLoader::RecordExtensionsMetrics() {
     base::UmaHistogramCounts100("Extensions.FileAccessNotAllowed",
                                 file_access_not_allowed_count);
   }
-  base::UmaHistogramCounts100("Extensions.CorruptExtensionTotalDisables",
-                              extension_prefs_->GetCorruptedDisableCount());
+  base::UmaHistogramCounts100(
+      "Extensions.CorruptExtensionTotalDisables",
+      extension_prefs_->GetPrefAsInteger(kCorruptedDisableCount));
   base::UmaHistogramCounts100("Extensions.EventlessEventPages",
                               eventless_event_pages_count);
   base::UmaHistogramCounts100("Extensions.LoadOffStoreItems",

extensions/browser/BUILD.gn

@@ -295,6 +295,8 @@ jumbo_source_set("browser_sources") {
     "policy_check.h",
     "pref_names.cc",
     "pref_names.h",
+    "pref_types.cc",
+    "pref_types.h",
     "preload_check.cc",
     "preload_check.h",
     "preload_check_group.cc",

extensions/browser/extension_prefs.cc

@@ -202,9 +202,6 @@ constexpr const char kExternalUninstalls[] = "extensions.external_uninstalls";
 // synced. Default value is false.
 constexpr const char kPrefDoNotSync[] = "do_not_sync";
 
-constexpr const char kCorruptedDisableCount[] =
-    "extensions.corrupted_disable_count";
-
 // A boolean preference that indicates whether the extension has local changes
 // that need to be synced. Default value is false.
 constexpr const char kPrefNeedsSync[] = "needs_sync";
@@ -240,6 +237,18 @@ constexpr const char kPrefDNRUseActionCountAsBadgeText[] =
 // installation or for extensions where the pref has not been set.
 constexpr bool kDefaultWithholdingBehavior = false;
 
+// Checks whether the value passed in is consistent with the expected PrefType.
+bool CheckPrefType(PrefType pref_type, const base::Value* value) {
+  switch (pref_type) {
+    case kBool:
+      return value->is_bool();
+    case kString:
+      return value->is_string();
+    case kInteger:
+      return value->is_int();
+  }
+}
+
 // Provider of write access to a dictionary storing extension prefs.
 class ScopedExtensionPrefUpdate : public prefs::ScopedDictionaryPrefUpdate {
  public:
@@ -1683,6 +1692,71 @@ void ExtensionPrefs::ClearLastLaunchTimes() {
   }
 }
 
+const base::Value* ExtensionPrefs::GetPref(const PrefMap& pref) const {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  const base::Value* pref_value = prefs_->Get(pref.name);
+  DCHECK(CheckPrefType(pref.type, pref_value))
+      << "PrefType does not match the value type of the stored value for "
+      << pref.name;
+  return pref_value;
+}
+
+void ExtensionPrefs::SetPref(const PrefMap& pref,
+                             std::unique_ptr<base::Value> value) {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK(CheckPrefType(pref.type, value.get()))
+      << "The value passed in does not match the expected PrefType for "
+      << pref.name;
+  prefs_->Set(pref.name, std::move(*value));
+}
+
+void ExtensionPrefs::SetIntegerPref(const PrefMap& pref, int value) {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK_EQ(PrefType::kInteger, pref.type);
+  prefs_->SetInteger(pref.name, value);
+}
+
+void ExtensionPrefs::SetBooleanPref(const PrefMap& pref, bool value) {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK_EQ(PrefType::kBool, pref.type);
+  prefs_->SetBoolean(pref.name, value);
+}
+
+void ExtensionPrefs::SetStringPref(const PrefMap& pref,
+                                   const std::string& value) {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK_EQ(PrefType::kString, pref.type);
+  prefs_->SetString(pref.name, value);
+}
+
+int ExtensionPrefs::GetPrefAsInteger(const PrefMap& pref) const {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK_EQ(PrefType::kInteger, pref.type);
+  return prefs_->GetInteger(pref.name);
+}
+
+bool ExtensionPrefs::GetPrefAsBoolean(const PrefMap& pref) const {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK_EQ(PrefType::kBool, pref.type);
+  return prefs_->GetBoolean(pref.name);
+}
+
+std::string ExtensionPrefs::GetPrefAsString(const PrefMap& pref) const {
+  DCHECK_EQ(PrefScope::kProfile, pref.scope);
+  DCHECK_EQ(PrefType::kString, pref.type);
+  return (prefs_->GetString(pref.name));
+}
+
+void ExtensionPrefs::IncrementPref(const PrefMap& pref) {
+  int count = GetPrefAsInteger(pref);
+  SetIntegerPref(pref, count + 1);
+}
+
+void ExtensionPrefs::DecrementPref(const PrefMap& pref) {
+  int count = GetPrefAsInteger(pref);
+  SetIntegerPref(pref, count - 1);
+}
+
 void ExtensionPrefs::GetExtensions(ExtensionIdList* out) const {
   CHECK(out);
 
@@ -1809,15 +1883,6 @@ void ExtensionPrefs::SetInstallParam(const std::string& extension_id,
                       std::make_unique<base::Value>(install_parameter));
 }
 
-int ExtensionPrefs::GetCorruptedDisableCount() const {
-  return prefs_->GetInteger(kCorruptedDisableCount);
-}
-
-void ExtensionPrefs::IncrementCorruptedDisableCount() {
-  int count = prefs_->GetInteger(kCorruptedDisableCount);
-  prefs_->SetInteger(kCorruptedDisableCount, count + 1);
-}
-
 bool ExtensionPrefs::NeedsSync(const std::string& extension_id) const {
   return ReadPrefAsBooleanAndReturn(extension_id, kPrefNeedsSync);
 }
@@ -2003,7 +2068,9 @@ void ExtensionPrefs::RegisterProfilePrefs(
   registry->RegisterListPref(pref_names::kNativeMessagingAllowlist);
   registry->RegisterBooleanPref(pref_names::kNativeMessagingUserLevelHosts,
                                 true);
-  registry->RegisterIntegerPref(kCorruptedDisableCount, 0);
+  // TODO(archanasimha): move pref registration to where the variable is
+  // defined.
+  registry->RegisterIntegerPref(kCorruptedDisableCount.name, 0);
 
 #if !defined(OS_MACOSX)
   registry->RegisterBooleanPref(pref_names::kAppFullscreenAllowed, true);

extensions/browser/extension_prefs.h

@@ -24,6 +24,7 @@
 #include "extensions/browser/disable_reason.h"
 #include "extensions/browser/extension_prefs_scope.h"
 #include "extensions/browser/install_flag.h"
+#include "extensions/browser/pref_types.h"
 #include "extensions/common/api/declarative_net_request/constants.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
@@ -238,6 +239,19 @@ class ExtensionPrefs : public KeyedService {
   BlocklistState GetExtensionBlocklistState(
       const std::string& extension_id) const;
 
+  // Gets or sets profile wide ExtensionPrefs.
+  void SetIntegerPref(const PrefMap& pref, int value);
+  void SetBooleanPref(const PrefMap& pref, bool value);
+  void SetStringPref(const PrefMap& pref, const std::string& value);
+
+  int GetPrefAsInteger(const PrefMap& pref) const;
+  bool GetPrefAsBoolean(const PrefMap& pref) const;
+  std::string GetPrefAsString(const PrefMap& pref) const;
+
+  // Increments/decrements an ExtensionPref with a PrefType::kInteger.
+  void IncrementPref(const PrefMap& pref);
+  void DecrementPref(const PrefMap& pref);
+
   // Populates |out| with the ids of all installed extensions.
   void GetExtensions(ExtensionIdList* out) const;
 
@@ -575,11 +589,6 @@ class ExtensionPrefs : public KeyedService {
   void SetInstallParam(const std::string& extension_id,
                        const std::string& install_parameter);
 
-  // The total number of times we've disabled an extension due to corrupted
-  // contents.
-  int GetCorruptedDisableCount() const;
-  void IncrementCorruptedDisableCount();
-
   // Whether the extension with the given |extension_id| needs to be synced.
   // This is set when the state (such as enabled/disabled or allowed in
   // incognito) is changed before Sync is ready.
@@ -686,6 +695,10 @@ class ExtensionPrefs : public KeyedService {
       bool extensions_disabled,
       const std::vector<EarlyExtensionPrefsObserver*>& early_observers);
 
+  // Gets or sets profile wide ExtensionPrefs.
+  const base::Value* GetPref(const PrefMap& pref) const;
+  void SetPref(const PrefMap& pref, std::unique_ptr<base::Value> value);
+
   // Converts absolute paths in the pref to paths relative to the
   // install_directory_.
   void MakePathsRelative();

extensions/browser/pref_types.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/browser/pref_types.h"
+
+namespace extensions {
+// Records the number of corrupted extensions that have been disabled.
+const PrefMap kCorruptedDisableCount = {"extensions.corrupted_disable_count",
+                                        PrefType::kInteger,
+                                        PrefScope::kProfile};
+}  // namespace extensions

extensions/browser/pref_types.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_BROWSER_PREF_TYPES_H_
+#define EXTENSIONS_BROWSER_PREF_TYPES_H_
+
+#include <string>
+
+namespace extensions {
+
+enum PrefType {
+  kBool,
+  kString,
+  kInteger
+  // TODO(archanasimha): implement Get/SetAsX for the following PrefTypes.
+  //  kGURL,
+  //  kList,
+  //  kDictionary,
+  //  kExtensionIdList,
+  //  kPermissionSet,
+  //  kTime
+};
+
+// PrefScope indicates whether an ExtensionPref is profile wide or specific to
+// an extension. Extension-specific prefs are keyed under a dictionary with the
+// extension ID and are removed when an extension is uninstalled.
+enum class PrefScope { kProfile, kExtensionSpecific };
+
+struct PrefMap {
+  const char* name;
+  PrefType type;
+  PrefScope scope;
+};
+
+extern const PrefMap kCorruptedDisableCount;
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_PREF_TYPES_H_