Allowing PrefService to add new PrefStores

When only running the service manager, a simple PrefService containing user
PrefStore and commandline PrefStore can be created before BrowserProcessImpl.
And it will be passed to the BrowserProcessImpl to create the local_state.
The implementation has already landed here:
https://chromium-review.googlesource.com/c/chromium/src/+/1148959
This CL makes it possible to migrate the simple PrefService to local state
by adding new pref stores, rather than creating the latter from scratch.
The benefit of doing this is that existing listeners to the simple PrefService
won't need to handle the case of swapping PrefServices,
re-registering PrefChangeRegistrar and default values, and verifying no
pref value changes during the swap.


Bug: 866028
Change-Id: I35b643d27ffabd9621f21f68a3261aa3482ca173
Reviewed-on: https://chromium-review.googlesource.com/1199944
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591469}

components/prefs/pref_notifier_impl.h

@@ -47,9 +47,11 @@ class COMPONENTS_PREFS_EXPORT PrefNotifierImpl : public PrefNotifier {
 
   void SetPrefService(PrefService* pref_service);
 
- protected:
   // PrefNotifier overrides.
   void OnPreferenceChanged(const std::string& pref_name) override;
+
+ protected:
+  // PrefNotifier overrides.
   void OnInitializationCompleted(bool succeeded) override;
 
   // A map from pref names to a list of observers. Observers get fired in the

components/prefs/pref_service.cc

@@ -5,6 +5,7 @@
 #include "components/prefs/pref_service.h"
 
 #include <algorithm>
+#include <map>
 #include <utility>
 
 #include "base/bind.h"
@@ -23,7 +24,6 @@
 #include "components/prefs/default_pref_store.h"
 #include "components/prefs/pref_notifier_impl.h"
 #include "components/prefs/pref_registry.h"
-#include "components/prefs/pref_value_store.h"
 
 namespace {
 
@@ -56,6 +56,29 @@ uint32_t GetWriteFlags(const PrefService::Preference* pref) {
   return write_flags;
 }
 
+// For prefs names in |pref_store| that are not presented in |pref_changed_map|,
+// check if their values differ from those in pref_service->FindPreference() and
+// add the result into |pref_changed_map|.
+void CheckForNewPrefChangesInPrefStore(
+    std::map<std::string, bool>* pref_changed_map,
+    PrefStore* pref_store,
+    PrefService* pref_service) {
+  if (!pref_store)
+    return;
+  auto values = pref_store->GetValues();
+  for (const auto& item : values->DictItems()) {
+    // If the key already presents, skip it as a store with higher precedence
+    // already sets the entry.
+    if (pref_changed_map->find(item.first) != pref_changed_map->end())
+      continue;
+    const PrefService::Preference* pref =
+        pref_service->FindPreference(item.first);
+    if (!pref)
+      continue;
+    pref_changed_map->emplace(item.first, *(pref->GetValue()) != item.second);
+  }
+}
+
 }  // namespace
 
 PrefService::PrefService(
@@ -393,6 +416,45 @@ void PrefService::OnStoreDeletionFromDisk() {
   user_pref_store_->OnStoreDeletionFromDisk();
 }
 
+void PrefService::ChangePrefValueStore(
+    PrefStore* managed_prefs,
+    PrefStore* supervised_user_prefs,
+    PrefStore* extension_prefs,
+    PrefStore* recommended_prefs,
+    std::unique_ptr<PrefValueStore::Delegate> delegate) {
+  // Only adding new pref stores are supported.
+  DCHECK(!pref_value_store_->HasPrefStore(PrefValueStore::MANAGED_STORE) ||
+         !managed_prefs);
+  DCHECK(
+      !pref_value_store_->HasPrefStore(PrefValueStore::SUPERVISED_USER_STORE) ||
+      !supervised_user_prefs);
+  DCHECK(!pref_value_store_->HasPrefStore(PrefValueStore::EXTENSION_STORE) ||
+         !extension_prefs);
+  DCHECK(!pref_value_store_->HasPrefStore(PrefValueStore::RECOMMENDED_STORE) ||
+         !recommended_prefs);
+
+  // If some of the stores are already initialized, check for pref value changes
+  // according to store precedence.
+  std::map<std::string, bool> pref_changed_map;
+  CheckForNewPrefChangesInPrefStore(&pref_changed_map, managed_prefs, this);
+  CheckForNewPrefChangesInPrefStore(&pref_changed_map, supervised_user_prefs,
+                                    this);
+  CheckForNewPrefChangesInPrefStore(&pref_changed_map, extension_prefs, this);
+  CheckForNewPrefChangesInPrefStore(&pref_changed_map, recommended_prefs, this);
+
+  pref_value_store_ = pref_value_store_->CloneAndSpecialize(
+      managed_prefs, supervised_user_prefs, extension_prefs,
+      nullptr /* command_line_prefs */, nullptr /* user_prefs */,
+      recommended_prefs, nullptr /* default_prefs */, pref_notifier_.get(),
+      std::move(delegate));
+
+  // Notify |pref_notifier_| on all changed values.
+  for (const auto& kv : pref_changed_map) {
+    if (kv.second)
+      pref_notifier_.get()->OnPreferenceChanged(kv.first);
+  }
+}
+
 void PrefService::AddPrefObserverAllPrefs(PrefObserver* obs) {
   pref_notifier_->AddPrefObserverAllPrefs(obs);
 }

components/prefs/pref_service.h

@@ -28,13 +28,13 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/prefs/persistent_pref_store.h"
+#include "components/prefs/pref_value_store.h"
 #include "components/prefs/prefs_export.h"
 
 class PrefNotifier;
 class PrefNotifierImpl;
 class PrefObserver;
 class PrefRegistry;
-class PrefValueStore;
 class PrefStore;
 
 namespace base {
@@ -337,6 +337,21 @@ class COMPONENTS_PREFS_EXPORT PrefService {
   // to tangentially cleanup data it may have saved outside the store.
   void OnStoreDeletionFromDisk();
 
+  // Add new pref stores to the existing PrefValueStore. Only adding new
+  // stores are allowed. If a corresponding store already exists, calling this
+  // will cause DCHECK failures. If the newly added stores already contain
+  // values, PrefNotifier associated with this object will be notified with
+  // these values. |delegate| can be passed to observe events of the new
+  // PrefValueStore.
+  // TODO(qinmin): packaging all the input params in a struct, and do the same
+  // for the constructor.
+  void ChangePrefValueStore(
+      PrefStore* managed_prefs,
+      PrefStore* supervised_user_prefs,
+      PrefStore* extension_prefs,
+      PrefStore* recommended_prefs,
+      std::unique_ptr<PrefValueStore::Delegate> delegate = nullptr);
+
   // A low level function for registering an observer for every single
   // preference changed notification. The caller must ensure that the observer
   // remains valid as long as it is registered. Pointer ownership is not
@@ -360,7 +375,7 @@ class COMPONENTS_PREFS_EXPORT PrefService {
 
   // The PrefValueStore provides prioritized preference values. It is owned by
   // this PrefService. Subclasses may access it for unit testing.
-  const std::unique_ptr<PrefValueStore> pref_value_store_;
+  std::unique_ptr<PrefValueStore> pref_value_store_;
 
   // Pref Stores and profile that we passed to the PrefValueStore.
   const scoped_refptr<PersistentPrefStore> user_pref_store_;

components/prefs/pref_service_factory.cc

@@ -40,3 +40,11 @@ std::unique_ptr<PrefService> PrefServiceFactory::Create(
       std::move(pref_notifier), std::move(pref_value_store), user_prefs_.get(),
       std::move(pref_registry), read_error_callback_, async_);
 }
+
+void PrefServiceFactory::ChangePrefValueStore(
+    PrefService* pref_service,
+    std::unique_ptr<PrefValueStore::Delegate> delegate) {
+  pref_service->ChangePrefValueStore(
+      managed_prefs_.get(), supervised_user_prefs_.get(),
+      extension_prefs_.get(), recommended_prefs_.get(), std::move(delegate));
+}

components/prefs/pref_service_factory.h

@@ -75,6 +75,11 @@ class COMPONENTS_PREFS_EXPORT PrefServiceFactory {
       scoped_refptr<PrefRegistry> pref_registry,
       std::unique_ptr<PrefValueStore::Delegate> delegate = nullptr);
 
+  // Add pref stores from this object to the |pref_service|.
+  void ChangePrefValueStore(
+      PrefService* pref_service,
+      std::unique_ptr<PrefValueStore::Delegate> delegate = nullptr);
+
  protected:
   scoped_refptr<PrefStore> managed_prefs_;
   scoped_refptr<PrefStore> supervised_user_prefs_;

components/prefs/pref_service_unittest.cc

@@ -13,6 +13,7 @@
 #include "components/prefs/json_pref_store.h"
 #include "components/prefs/mock_pref_change_callback.h"
 #include "components/prefs/pref_change_registrar.h"
+#include "components/prefs/pref_notifier_impl.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service_factory.h"
 #include "components/prefs/pref_value_store.h"
@@ -24,7 +25,14 @@
 using testing::_;
 using testing::Mock;
 
+namespace {
+
 const char kPrefName[] = "pref.name";
+const char kManagedPref[] = "managed_pref";
+const char kRecommendedPref[] = "recommended_pref";
+const char kSupervisedPref[] = "supervised_pref";
+
+}  // namespace
 
 TEST(PrefServiceTest, NoObserverFire) {
   TestingPrefServiceSimple prefs;
@@ -486,3 +494,131 @@ TEST_F(PrefServiceSetValueTest, SetListValue) {
   prefs_.Set(kName, empty);
   Mock::VerifyAndClearExpectations(&observer_);
 }
+
+class PrefValueStoreChangeTest : public testing::Test {
+ protected:
+  PrefValueStoreChangeTest()
+      : user_pref_store_(base::MakeRefCounted<TestingPrefStore>()),
+        pref_registry_(base::MakeRefCounted<PrefRegistrySimple>()) {}
+
+  ~PrefValueStoreChangeTest() override = default;
+
+  void SetUp() override {
+    auto pref_notifier = std::make_unique<PrefNotifierImpl>();
+    auto pref_value_store = std::make_unique<PrefValueStore>(
+        nullptr /* managed_prefs */, nullptr /* supervised_user_prefs */,
+        nullptr /* extension_prefs */, new TestingPrefStore(),
+        user_pref_store_.get(), nullptr /* recommended_prefs */,
+        pref_registry_->defaults().get(), pref_notifier.get());
+    pref_service_ = std::make_unique<PrefService>(
+        std::move(pref_notifier), std::move(pref_value_store), user_pref_store_,
+        pref_registry_, base::DoNothing(), false);
+    pref_registry_->RegisterIntegerPref(kManagedPref, 1);
+    pref_registry_->RegisterIntegerPref(kRecommendedPref, 2);
+    pref_registry_->RegisterIntegerPref(kSupervisedPref, 3);
+  }
+
+  std::unique_ptr<PrefService> pref_service_;
+  scoped_refptr<TestingPrefStore> user_pref_store_;
+  scoped_refptr<PrefRegistrySimple> pref_registry_;
+};
+
+// Check that value from the new PrefValueStore will be correctly retrieved.
+TEST_F(PrefValueStoreChangeTest, ChangePrefValueStore) {
+  const PrefService::Preference* preference =
+      pref_service_->FindPreference(kManagedPref);
+  EXPECT_TRUE(preference->IsDefaultValue());
+  EXPECT_EQ(base::Value(1), *(preference->GetValue()));
+  const PrefService::Preference* supervised =
+      pref_service_->FindPreference(kSupervisedPref);
+  EXPECT_TRUE(supervised->IsDefaultValue());
+  EXPECT_EQ(base::Value(3), *(supervised->GetValue()));
+  const PrefService::Preference* recommended =
+      pref_service_->FindPreference(kRecommendedPref);
+  EXPECT_TRUE(recommended->IsDefaultValue());
+  EXPECT_EQ(base::Value(2), *(recommended->GetValue()));
+
+  user_pref_store_->SetInteger(kManagedPref, 10);
+  EXPECT_TRUE(preference->IsUserControlled());
+  ASSERT_EQ(base::Value(10), *(preference->GetValue()));
+
+  scoped_refptr<TestingPrefStore> managed_pref_store =
+      base::MakeRefCounted<TestingPrefStore>();
+  pref_service_->ChangePrefValueStore(
+      managed_pref_store.get(), nullptr /* supervised_user_prefs */,
+      nullptr /* extension_prefs */, nullptr /* recommended_prefs */);
+  EXPECT_TRUE(preference->IsUserControlled());
+  ASSERT_EQ(base::Value(10), *(preference->GetValue()));
+
+  // Test setting a managed pref after overriding the managed PrefStore.
+  managed_pref_store->SetInteger(kManagedPref, 20);
+  EXPECT_TRUE(preference->IsManaged());
+  ASSERT_EQ(base::Value(20), *(preference->GetValue()));
+
+  // Test overriding the supervised and recommended PrefStore with already set
+  // prefs.
+  scoped_refptr<TestingPrefStore> supervised_pref_store =
+      base::MakeRefCounted<TestingPrefStore>();
+  scoped_refptr<TestingPrefStore> recommened_pref_store =
+      base::MakeRefCounted<TestingPrefStore>();
+  supervised_pref_store->SetInteger(kManagedPref, 30);
+  supervised_pref_store->SetInteger(kSupervisedPref, 31);
+  recommened_pref_store->SetInteger(kManagedPref, 40);
+  recommened_pref_store->SetInteger(kRecommendedPref, 41);
+  pref_service_->ChangePrefValueStore(
+      nullptr /* managed_prefs */, supervised_pref_store.get(),
+      nullptr /* extension_prefs */, recommened_pref_store.get());
+  EXPECT_TRUE(preference->IsManaged());
+  ASSERT_EQ(base::Value(20), *(preference->GetValue()));
+  EXPECT_TRUE(supervised->IsManagedByCustodian());
+  EXPECT_EQ(base::Value(31), *(supervised->GetValue()));
+  EXPECT_TRUE(recommended->IsRecommended());
+  EXPECT_EQ(base::Value(41), *(recommended->GetValue()));
+}
+
+// Tests that PrefChangeRegistrar works after PrefValueStore is changed.
+TEST_F(PrefValueStoreChangeTest, PrefChangeRegistrar) {
+  MockPrefChangeCallback obs(pref_service_.get());
+  PrefChangeRegistrar registrar;
+  registrar.Init(pref_service_.get());
+  registrar.Add(kManagedPref, obs.GetCallback());
+  registrar.Add(kSupervisedPref, obs.GetCallback());
+  registrar.Add(kRecommendedPref, obs.GetCallback());
+
+  base::Value expected_value(10);
+  obs.Expect(kManagedPref, &expected_value);
+  user_pref_store_->SetInteger(kManagedPref, 10);
+  Mock::VerifyAndClearExpectations(&obs);
+  expected_value = base::Value(11);
+  obs.Expect(kRecommendedPref, &expected_value);
+  user_pref_store_->SetInteger(kRecommendedPref, 11);
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Test overriding the managed and supervised PrefStore with already set
+  // prefs.
+  scoped_refptr<TestingPrefStore> managed_pref_store =
+      base::MakeRefCounted<TestingPrefStore>();
+  scoped_refptr<TestingPrefStore> supervised_pref_store =
+      base::MakeRefCounted<TestingPrefStore>();
+  // Update |kManagedPref| before changing the PrefValueStore, the
+  // PrefChangeRegistrar should get notified on |kManagedPref| as its value
+  // changes.
+  managed_pref_store->SetInteger(kManagedPref, 20);
+  // Due to store precedence, the value of |kRecommendedPref| will not be
+  // changed so PrefChangeRegistrar will not be notified.
+  managed_pref_store->SetInteger(kRecommendedPref, 11);
+  supervised_pref_store->SetInteger(kManagedPref, 30);
+  supervised_pref_store->SetInteger(kRecommendedPref, 21);
+  expected_value = base::Value(20);
+  obs.Expect(kManagedPref, &expected_value);
+  pref_service_->ChangePrefValueStore(
+      managed_pref_store.get(), supervised_pref_store.get(),
+      nullptr /* extension_prefs */, nullptr /* recommended_prefs */);
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Update a pref value after PrefValueStore change, it should also work.
+  expected_value = base::Value(31);
+  obs.Expect(kSupervisedPref, &expected_value);
+  supervised_pref_store->SetInteger(kSupervisedPref, 31);
+  Mock::VerifyAndClearExpectations(&obs);
+}

components/prefs/pref_value_store.cc

@@ -205,6 +205,10 @@ bool PrefValueStore::IsInitializationComplete() const {
   return true;
 }
 
+bool PrefValueStore::HasPrefStore(PrefStoreType type) const {
+  return GetPrefStore(type);
+}
+
 bool PrefValueStore::PrefValueInStore(
     const std::string& name,
     PrefValueStore::PrefStoreType store) const {

components/prefs/pref_value_store.h

@@ -180,6 +180,9 @@ class COMPONENTS_PREFS_EXPORT PrefValueStore {
 
   bool IsInitializationComplete() const;
 
+  // Check whether a particular type of PrefStore exists.
+  bool HasPrefStore(PrefStoreType type) const;
+
  private:
   // Keeps a PrefStore reference on behalf of the PrefValueStore and monitors
   // the PrefStore for changes, forwarding notifications to PrefValueStore. This