Change prefs::ConnectToPrefService() to take local pref stores.

Currently, ConnectToPrefService() takes a list of pref store types that
are to be provided locally, but does not expose a way to register them
with the created PrefService object. This replaces that with a map of
pref stores that are local.

Bug: 654988
Change-Id: I187bf1a148e3e2e041e6f3b01b7eefc4deaabefd
Reviewed-on: https://chromium-review.googlesource.com/514942
Commit-Queue: Sam McNally <sammc@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Johan Tibell <tibell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474964}

ash/shell.cc

@@ -807,8 +807,7 @@ void Shell::Init(const ShellInitParams& init_params) {
     auto pref_registry = base::MakeRefCounted<PrefRegistrySimple>();
     Shell::RegisterPrefs(pref_registry.get());
     prefs::ConnectToPrefService(
-        shell_delegate_->GetShellConnector(), std::move(pref_registry),
-        std::vector<PrefValueStore::PrefStoreType>(),
+        shell_delegate_->GetShellConnector(), std::move(pref_registry), {},
         base::Bind(&Shell::OnPrefServiceInitialized, base::Unretained(this)),
         prefs::mojom::kForwarderServiceName);
   }

services/preferences/pref_service_factory_unittest.cc

@@ -119,25 +119,38 @@ class PrefServiceFactoryTest : public service_manager::test::ServiceTest {
   }
 
   // Create a fully initialized PrefService synchronously.
-  std::unique_ptr<PrefService> Create() {
+  std::unique_ptr<PrefService> Create() { return CreateImpl({}); }
+
+  std::unique_ptr<PrefService> CreateWithLocalLayeredPrefStores() {
+    return CreateImpl(
+        {{
+             {PrefValueStore::COMMAND_LINE_STORE, above_user_prefs_pref_store_},
+             {PrefValueStore::RECOMMENDED_STORE, below_user_prefs_pref_store_},
+         },
+         base::KEEP_LAST_OF_DUPES});
+  }
+
+  std::unique_ptr<PrefService> CreateImpl(
+      base::flat_map<PrefValueStore::PrefStoreType, scoped_refptr<PrefStore>>
+          local_pref_stores) {
     std::unique_ptr<PrefService> pref_service;
     base::RunLoop run_loop;
-    CreateAsync(std::vector<PrefValueStore::PrefStoreType>(),
-                run_loop.QuitClosure(), &pref_service);
+    CreateAsync(std::move(local_pref_stores), run_loop.QuitClosure(),
+                &pref_service);
     run_loop.Run();
     return pref_service;
   }
 
-  void CreateAsync(
-      const std::vector<PrefValueStore::PrefStoreType>& already_connected_types,
-      base::Closure callback,
-      std::unique_ptr<PrefService>* out) {
+  void CreateAsync(base::flat_map<PrefValueStore::PrefStoreType,
+                                  scoped_refptr<PrefStore>> local_pref_stores,
+                   base::Closure callback,
+                   std::unique_ptr<PrefService>* out) {
     auto pref_registry = make_scoped_refptr(new PrefRegistrySimple());
     pref_registry->RegisterIntegerPref(kKey, kInitialValue);
     pref_registry->RegisterIntegerPref(kOtherKey, kInitialValue);
     pref_registry->RegisterDictionaryPref(kDictionaryKey);
     ConnectToPrefService(
-        connector(), pref_registry, already_connected_types,
+        connector(), std::move(pref_registry), std::move(local_pref_stores),
         base::Bind(&PrefServiceFactoryTest::OnCreate, callback, out));
   }
 
@@ -238,6 +251,22 @@ TEST_F(PrefServiceFactoryTest, ReadOnlyPrefStore) {
   EXPECT_EQ(4, pref_service->GetInteger(kKey));
 }
 
+// Check that local read-only pref store changes are observed.
+TEST_F(PrefServiceFactoryTest, ReadOnlyPrefStore_Local) {
+  auto pref_service = CreateWithLocalLayeredPrefStores();
+
+  EXPECT_EQ(kInitialValue, pref_service->GetInteger(kKey));
+
+  below_user_prefs_pref_store()->SetValue(
+      kKey, base::MakeUnique<base::Value>(kUpdatedValue), 0);
+  EXPECT_EQ(kUpdatedValue, pref_service->GetInteger(kKey));
+  pref_service->SetInteger(kKey, 3);
+  EXPECT_EQ(3, pref_service->GetInteger(kKey));
+  above_user_prefs_pref_store()->SetValue(kKey,
+                                          base::MakeUnique<base::Value>(4), 0);
+  EXPECT_EQ(4, pref_service->GetInteger(kKey));
+}
+
 // Check that updates to read-only pref stores are correctly layered.
 TEST_F(PrefServiceFactoryTest, ReadOnlyPrefStore_Layering) {
   auto pref_service = Create();
@@ -543,8 +572,7 @@ TEST_F(PrefServiceFactoryManualPrefStoreRegistrationTest,
   std::unique_ptr<PrefService> pref_service;
   base::RunLoop run_loop;
   auto barrier = base::BarrierClosure(2, run_loop.QuitClosure());
-  CreateAsync(std::vector<PrefValueStore::PrefStoreType>(), barrier,
-              &pref_service);
+  CreateAsync({}, barrier, &pref_service);
 
   add_observer_run_loop.Run();
   ASSERT_TRUE(below_user_prefs.observer_added());
@@ -568,14 +596,7 @@ TEST_F(PrefServiceFactoryManualPrefStoreRegistrationTest,
 // do not wait for those stores to be registered with the pref service.
 TEST_F(PrefServiceFactoryManualPrefStoreRegistrationTest,
        LocalButNotRegisteredReadOnlyStores) {
-  std::unique_ptr<PrefService> pref_service;
-  base::RunLoop run_loop;
-  CreateAsync(
-      {PrefValueStore::RECOMMENDED_STORE, PrefValueStore::COMMAND_LINE_STORE},
-      run_loop.QuitClosure(), &pref_service);
-
-  run_loop.Run();
-  EXPECT_TRUE(pref_service);
+  EXPECT_TRUE(CreateWithLocalLayeredPrefStores());
 }
 
 }  // namespace

services/preferences/public/cpp/pref_service_factory.cc

@@ -41,34 +41,45 @@ void DoNothingHandleReadError(PersistentPrefStore::PrefReadError error) {}
 scoped_refptr<PrefStore> CreatePrefStoreClient(
     PrefValueStore::PrefStoreType store_type,
     std::unordered_map<PrefValueStore::PrefStoreType,
-                       mojom::PrefStoreConnectionPtr>* connections) {
+                       mojom::PrefStoreConnectionPtr>* connections,
+    base::flat_map<PrefValueStore::PrefStoreType, scoped_refptr<PrefStore>>*
+        local_layered_pref_stores) {
+  auto local_pref_store_it = local_layered_pref_stores->find(store_type);
+  if (local_pref_store_it != local_layered_pref_stores->end()) {
+    return std::move(local_pref_store_it->second);
+  }
   auto pref_store_it = connections->find(store_type);
   if (pref_store_it != connections->end()) {
-    return make_scoped_refptr(
-        new PrefStoreClient(std::move(pref_store_it->second)));
-  } else {
-    return nullptr;
+    return base::MakeRefCounted<PrefStoreClient>(
+        std::move(pref_store_it->second));
   }
+  return nullptr;
 }
 
 void OnConnect(
     scoped_refptr<RefCountedInterfacePtr<mojom::PrefStoreConnector>>
         connector_ptr,
     scoped_refptr<PrefRegistry> pref_registry,
+    base::flat_map<PrefValueStore::PrefStoreType, scoped_refptr<PrefStore>>
+        local_layered_pref_stores,
     ConnectCallback callback,
     mojom::PersistentPrefStoreConnectionPtr persistent_pref_store_connection,
     std::unordered_map<PrefValueStore::PrefStoreType,
                        mojom::PrefStoreConnectionPtr> connections) {
-  scoped_refptr<PrefStore> managed_prefs =
-      CreatePrefStoreClient(PrefValueStore::MANAGED_STORE, &connections);
-  scoped_refptr<PrefStore> supervised_user_prefs = CreatePrefStoreClient(
-      PrefValueStore::SUPERVISED_USER_STORE, &connections);
+  scoped_refptr<PrefStore> managed_prefs = CreatePrefStoreClient(
+      PrefValueStore::MANAGED_STORE, &connections, &local_layered_pref_stores);
+  scoped_refptr<PrefStore> supervised_user_prefs =
+      CreatePrefStoreClient(PrefValueStore::SUPERVISED_USER_STORE, &connections,
+                            &local_layered_pref_stores);
   scoped_refptr<PrefStore> extension_prefs =
-      CreatePrefStoreClient(PrefValueStore::EXTENSION_STORE, &connections);
+      CreatePrefStoreClient(PrefValueStore::EXTENSION_STORE, &connections,
+                            &local_layered_pref_stores);
   scoped_refptr<PrefStore> command_line_prefs =
-      CreatePrefStoreClient(PrefValueStore::COMMAND_LINE_STORE, &connections);
+      CreatePrefStoreClient(PrefValueStore::COMMAND_LINE_STORE, &connections,
+                            &local_layered_pref_stores);
   scoped_refptr<PrefStore> recommended_prefs =
-      CreatePrefStoreClient(PrefValueStore::RECOMMENDED_STORE, &connections);
+      CreatePrefStoreClient(PrefValueStore::RECOMMENDED_STORE, &connections,
+                            &local_layered_pref_stores);
   // TODO(crbug.com/719770): Once owning registrations are supported, pass the
   // default values of owned prefs to the service and connect to the service's
   // defaults pref store instead of using a local one.
@@ -99,9 +110,15 @@ void OnConnectError(
 void ConnectToPrefService(
     service_manager::Connector* connector,
     scoped_refptr<PrefRegistry> pref_registry,
-    std::vector<PrefValueStore::PrefStoreType> already_connected_types,
+    base::flat_map<PrefValueStore::PrefStoreType, scoped_refptr<PrefStore>>
+        local_layered_pref_stores,
     ConnectCallback callback,
     base::StringPiece service_name) {
+  std::vector<PrefValueStore::PrefStoreType> already_connected_types;
+  already_connected_types.reserve(local_layered_pref_stores.size());
+  for (const auto& store : local_layered_pref_stores) {
+    already_connected_types.push_back(store.first);
+  }
   already_connected_types.push_back(PrefValueStore::DEFAULT_STORE);
   auto connector_ptr = make_scoped_refptr(
       new RefCountedInterfacePtr<mojom::PrefStoreConnector>());
@@ -112,6 +129,7 @@ void ConnectToPrefService(
   connector_ptr->get()->Connect(
       std::move(serialized_pref_registry), std::move(already_connected_types),
       base::Bind(&OnConnect, connector_ptr, base::Passed(&pref_registry),
+                 base::Passed(&local_layered_pref_stores),
                  base::Passed(&callback)));
 }
 

services/preferences/public/cpp/pref_service_factory.h

@@ -39,7 +39,8 @@ using ConnectCallback = base::Callback<void(std::unique_ptr<::PrefService>)>;
 void ConnectToPrefService(
     service_manager::Connector* connector,
     scoped_refptr<PrefRegistry> pref_registry,
-    std::vector<PrefValueStore::PrefStoreType> already_connected_types,
+    base::flat_map<PrefValueStore::PrefStoreType, scoped_refptr<PrefStore>>
+        local_layered_pref_stores,
     ConnectCallback callback,
     base::StringPiece service_name = mojom::kServiceName);