Pref service: Filter updates from read-only pref stores.

services/preferences/public/cpp/tests/pref_store_impl_unittest.cc

Index: services/preferences/public/cpp/tests/pref_store_impl_unittest.cc
diff --git a/services/preferences/public/cpp/tests/pref_store_impl_unittest.cc b/services/preferences/public/cpp/tests/pref_store_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0e30747604a228ea0e3d5a7d04aaf038622d9e52
--- /dev/null
+++ b/services/preferences/public/cpp/tests/pref_store_impl_unittest.cc
@@ -0,0 +1,336 @@
+// Copyright 2017 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 "services/preferences/public/cpp/pref_store_impl.h"
+
+#include <utility>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/values.h"
+#include "components/prefs/value_map_pref_store.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
+#include "services/preferences/public/cpp/pref_store_client.h"
+#include "services/preferences/public/interfaces/preferences.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Invoke;
+using testing::WithoutArgs;
+
+namespace prefs {
+namespace {
+
+class PrefStoreObserverMock : public PrefStore::Observer {
+ public:
+  MOCK_METHOD1(OnPrefValueChanged, void(const std::string&));
+  MOCK_METHOD1(OnInitializationCompleted, void(bool succeeded));

[tibell, 2017/03/27 03:37:56]

Can drop |succeeded| here for consistency with the above method.

[Sam McNally, 2017/03/27 06:44:40]

Done.

+};
+
+class MockPrefStore : public ValueMapPrefStore {
+ public:
+  bool IsInitializationComplete() const override {
+    return initialized_ && success_;
+  }
+
+  void AddObserver(PrefStore::Observer* observer) override {
+    observers_.AddObserver(observer);
+  }
+
+  void RemoveObserver(PrefStore::Observer* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
+  void CompleteInitialization(bool success) {
+    initialized_ = true;
+    success_ = success;
+    for (auto& observer : observers_) {
+      // Some pref stores report completing initialization more than once. Test
+      // that additional calls are ignored.
+      observer.OnInitializationCompleted(success);
+      observer.OnInitializationCompleted(success);
+    }
+  }
+
+ private:
+  ~MockPrefStore() override = default;
+
+  bool initialized_ = false;
+  bool success_ = false;
+  base::ObserverList<PrefStore::Observer, true> observers_;
+};
+
+constexpr char kKey[] = "path.to.key";
+constexpr char kOtherKey[] = "path.to.other_key";
+
+class PrefStoreImplTest : public testing::Test {
+ public:
+  PrefStoreImplTest() = default;
+
+  // testing::Test:
+  void TearDown() override {
+    pref_store_ = nullptr;
+    base::RunLoop().RunUntilIdle();
+    pref_store_ptr_.reset();
+    base::RunLoop().RunUntilIdle();
+    impl_.reset();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void CreateImpl(scoped_refptr<PrefStore> backing_pref_store,

[tibell, 2017/03/27 03:37:56]

Could this just return the PrefStore instead? Less state to worry about in the
test.

[Sam McNally, 2017/03/27 06:44:40]

Different tests use different PrefStores.

+                  std::vector<std::string> observed_prefs = {}) {
+    impl_ = base::MakeUnique<PrefStoreImpl>(
+        std::move(backing_pref_store), mojo::MakeRequest(&pref_store_ptr_));
+
+    if (observed_prefs.empty())
+      observed_prefs.insert(observed_prefs.end(), {kKey, kOtherKey});
+    pref_store_ = CreateConnection(std::move(observed_prefs));
+  }
+
+  scoped_refptr<PrefStore> CreateConnection(
+      std::vector<std::string> observed_prefs) {
+    base::RunLoop run_loop;
+    mojom::PrefStoreConnectionPtr connection;
+    pref_store_ptr_->AddObserver(
+        observed_prefs,
+        base::Bind(
+            [](mojom::PrefStoreConnectionPtr* output, base::OnceClosure quit,
+               mojom::PrefStoreConnectionPtr connection) {
+              std::move(quit).Run();
+              *output = std::move(connection);
+            },
+            &connection, run_loop.QuitClosure()));
+    run_loop.Run();
+    return make_scoped_refptr(new PrefStoreClient(std::move(connection)));
+  }
+
+  PrefStore* pref_store() { return pref_store_.get(); }
+
+ private:
+  base::MessageLoop message_loop_;
+
+  std::unique_ptr<PrefStoreImpl> impl_;
+  mojom::PrefStorePtr pref_store_ptr_;
+
+  scoped_refptr<PrefStore> pref_store_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefStoreImplTest);
+};
+
+TEST_F(PrefStoreImplTest, InitializationSuccess) {
+  auto backing_pref_store = make_scoped_refptr(new MockPrefStore());
+  CreateImpl(backing_pref_store);
+  EXPECT_FALSE(pref_store()->IsInitializationComplete());
+
+  backing_pref_store->CompleteInitialization(true);
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnInitializationCompleted(true))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Times(1) is the default so you can remove.

[Sam McNally, 2017/03/27 06:44:40]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+  EXPECT_TRUE(pref_store()->IsInitializationComplete());
+}
+
+TEST_F(PrefStoreImplTest, InitializationFailure) {
+  auto backing_pref_store = make_scoped_refptr(new MockPrefStore());
+  CreateImpl(backing_pref_store);
+  EXPECT_FALSE(pref_store()->IsInitializationComplete());
+
+  backing_pref_store->CompleteInitialization(false);
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnInitializationCompleted(false))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

ditto

[Sam McNally, 2017/03/27 06:44:41]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+
+  // TODO(sammc): Should IsInitializationComplete() return false?
+  EXPECT_TRUE(pref_store()->IsInitializationComplete());
+}
+
+TEST_F(PrefStoreImplTest, InitialValue) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  const base::Value value("value");
+  backing_pref_store->SetValue(kKey, value.CreateDeepCopy(), 0);
+  CreateImpl(backing_pref_store);
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());

[tibell, 2017/03/27 03:37:56]

Maybe add a TODO here too to check what the backing pref store does in this case
(i.e. is having values enough to be considered initialized)?

[Sam McNally, 2017/03/27 06:44:41]

As discussed, this isn't testing that the presence of values causes this to be
initialized; ValueMapPrefStore is always initialized so the client should also
always be initialized.

+  const base::Value* output = nullptr;
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  EXPECT_TRUE(value.Equals(output));
+}
+
+TEST_F(PrefStoreImplTest, InitialValueWithoutPathExpansion) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  base::DictionaryValue dict;
+  dict.SetStringWithoutPathExpansion(kKey, "value");
+  backing_pref_store->SetValue(kKey, dict.CreateDeepCopy(), 0);
+  CreateImpl(backing_pref_store);
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+  const base::Value* output = nullptr;
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  EXPECT_TRUE(dict.Equals(output));
+}
+
+TEST_F(PrefStoreImplTest, WriteObservedByClient) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  CreateImpl(backing_pref_store);
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+
+  const base::Value value("value");
+  backing_pref_store->SetValue(kKey, value.CreateDeepCopy(), 0);
+
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnPrefValueChanged(kKey))

[tibell, 2017/03/27 03:37:56]

Could this be lifted out into a

ExpectPrefChange(PrefStore, const string&)

helper?

[Sam McNally, 2017/03/27 06:44:40]

Done.

+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Remove

[Sam McNally, 2017/03/27 06:44:40]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+
+  const base::Value* output = nullptr;
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  EXPECT_TRUE(value.Equals(output));
+}
+
+TEST_F(PrefStoreImplTest, WriteToUnregisteredPrefNotObservedByClient) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  CreateImpl(backing_pref_store, {kKey});
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+
+  backing_pref_store->SetValue(kOtherKey, base::MakeUnique<base::Value>(123),
+                               0);
+  backing_pref_store->SetValue(kKey, base::MakeUnique<base::Value>("value"), 0);
+
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnPrefValueChanged(kOtherKey)).Times(0);
+  EXPECT_CALL(observer, OnPrefValueChanged(kKey))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Remove

[Sam McNally, 2017/03/27 06:44:41]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+
+  EXPECT_FALSE(pref_store()->GetValue(kOtherKey, nullptr));
+}
+
+TEST_F(PrefStoreImplTest, WriteWithoutPathExpansionObservedByClient) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  CreateImpl(backing_pref_store);
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+
+  base::DictionaryValue dict;
+  dict.SetStringWithoutPathExpansion(kKey, "value");
+  backing_pref_store->SetValue(kKey, dict.CreateDeepCopy(), 0);
+
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnPrefValueChanged(kKey))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Remove

[Sam McNally, 2017/03/27 06:44:40]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+
+  const base::Value* output = nullptr;
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  EXPECT_TRUE(dict.Equals(output));
+}
+
+TEST_F(PrefStoreImplTest, RemoveObservedByClient) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  const base::Value value("value");
+  backing_pref_store->SetValue(kKey, value.CreateDeepCopy(), 0);
+  CreateImpl(backing_pref_store);
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+
+  const base::Value* output = nullptr;
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  EXPECT_TRUE(value.Equals(output));
+  backing_pref_store->RemoveValue(kKey, 0);
+
+  // This should be a no-op and shouldn't trigger a notification for the other
+  // client.
+  backing_pref_store->RemoveValue(kKey, 0);
+
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnPrefValueChanged(kKey))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Remove

[Sam McNally, 2017/03/27 06:44:41]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  base::RunLoop().RunUntilIdle();
+  pref_store()->RemoveObserver(&observer);
+
+  EXPECT_FALSE(pref_store()->GetValue(kKey, &output));
+}
+
+TEST_F(PrefStoreImplTest, RemoveOfUnregisteredPrefNotObservedByClient) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  const base::Value value("value");
+  backing_pref_store->SetValue(kKey, value.CreateDeepCopy(), 0);
+  backing_pref_store->SetValue(kOtherKey, value.CreateDeepCopy(), 0);
+  CreateImpl(backing_pref_store, {kKey});
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+
+  backing_pref_store->RemoveValue(kOtherKey, 0);
+  backing_pref_store->RemoveValue(kKey, 0);
+
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnPrefValueChanged(kOtherKey)).Times(0);
+  EXPECT_CALL(observer, OnPrefValueChanged(kKey))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Remove

[Sam McNally, 2017/03/27 06:44:40]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+}
+
+TEST_F(PrefStoreImplTest, RemoveWithoutPathExpansionObservedByOtherClient) {
+  auto backing_pref_store = make_scoped_refptr(new ValueMapPrefStore());
+  base::DictionaryValue dict;
+  dict.SetStringWithoutPathExpansion(kKey, "value");
+  backing_pref_store->SetValue(kKey, dict.CreateDeepCopy(), 0);
+  CreateImpl(backing_pref_store);
+  ASSERT_TRUE(pref_store()->IsInitializationComplete());
+
+  const base::Value* output = nullptr;
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  EXPECT_TRUE(dict.Equals(output));
+
+  base::Value* mutable_value = nullptr;
+  dict.SetStringWithoutPathExpansion(kKey, "value");
+  ASSERT_TRUE(backing_pref_store->GetMutableValue(kKey, &mutable_value));
+  base::DictionaryValue* mutable_dict = nullptr;
+  ASSERT_TRUE(mutable_value->GetAsDictionary(&mutable_dict));
+  mutable_dict->RemoveWithoutPathExpansion(kKey, nullptr);
+  backing_pref_store->ReportValueChanged(kKey, 0);
+
+  PrefStoreObserverMock observer;
+  pref_store()->AddObserver(&observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(observer, OnPrefValueChanged(kKey))
+      .Times(1)

[tibell, 2017/03/27 03:37:56]

Remove

[Sam McNally, 2017/03/27 06:44:40]

Done.

+      .WillOnce(WithoutArgs(Invoke([&run_loop]() { run_loop.Quit(); })));
+  run_loop.Run();
+  pref_store()->RemoveObserver(&observer);
+
+  ASSERT_TRUE(pref_store()->GetValue(kKey, &output));
+  const base::DictionaryValue* dict_value = nullptr;
+  ASSERT_TRUE(output->GetAsDictionary(&dict_value));
+  EXPECT_TRUE(dict_value->empty());
+}
+
+}  // namespace
+}  // namespace prefs