Implement lossy pref behavior for JsonPrefStore.

base/prefs/json_pref_store_unittest.cc

Index: base/prefs/json_pref_store_unittest.cc
diff --git a/base/prefs/json_pref_store_unittest.cc b/base/prefs/json_pref_store_unittest.cc
index 0fab8685e9b21410731955169bad35b5d98ceef2..4f4bf8f2e69235a9ce68018dc003862bc261d215 100644
--- a/base/prefs/json_pref_store_unittest.cc
+++ b/base/prefs/json_pref_store_unittest.cc
@@ -213,7 +213,7 @@ void RunBasicJsonPrefStoreTest(JsonPrefStore* pref_store,
   EXPECT_FALSE(boolean);
 
   EXPECT_TRUE(pref_store->GetValue(kMaxTabs, &actual));
-  int integer = 0;
+  int integer;
   EXPECT_TRUE(actual->GetAsInteger(&integer));
   EXPECT_EQ(20, integer);
   pref_store->SetValue(kMaxTabs, new FundamentalValue(10),
@@ -809,4 +809,160 @@ TEST_F(JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps) {
   ASSERT_EQ(6, samples->TotalCount());
 }
 
+class TestImportantFileWriter : public ImportantFileWriter {
+ public:
+  TestImportantFileWriter() {}
+  ~TestImportantFileWriter() override {}
+
+  // ImportantFileWriter overrides.
+  bool HasPendingWrite() const override { return !!serializer_; }
+
+  void WriteNow(scoped_ptr<std::string> data) override { NOTREACHED(); }
+
+  void ScheduleWrite(DataSerializer* serializer) override {
+    serializer_ = serializer;
+  }
+
+  void DoScheduledWrite() override {
+    CHECK(serializer_ && !has_last_write_);
+    if (!serializer_->SerializeData(&last_write_))
+      NOTREACHED();
+    serializer_ = nullptr;
+    has_last_write_ = true;
+  }
+
+  void RegisterOnNextSuccessfulWriteCallback(
+      const base::Closure& on_next_successful_write) override {
+    NOTREACHED();
+  }
+
+  TimeDelta CommitInterval() const override {
+    return TimeDelta::FromSeconds(5);
+  }
+
+  std::string GetAndClearLastWrite() {
+    std::string result = last_write_;
+    last_write_ = std::string();
+    has_last_write_ = false;
+    return result;
+  }
+
+ private:
+  DataSerializer* serializer_ = nullptr;
+  std::string last_write_;
+  bool has_last_write_ = false;
+};
+
+// This test subclass just contains some helper functions.
+class JsonPrefStoreLossyWriteTest : public JsonPrefStoreTest {
+ protected:
+  // A struct used for conveniently setting values in the PrefStore
+  struct PrefDetails {

[Mattias Nissler (ping if slow), 2015/05/07 07:59:53]

This is actually not providing any benefit that I can see? You could just as
well inline the constants in the test code below.

+    std::string key;
+    scoped_ptr<base::Value> value;
+    uint32 flags;
+  };
+
+  static PrefDetails GetNormalPrefDetails() {
+    return {"normal",
+            scoped_ptr<base::Value>(new base::StringValue("normal")),
+            WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS};
+  }
+
+  static PrefDetails GetLossyPrefDetails() {
+    return {"lossy",
+            scoped_ptr<base::Value>(new base::StringValue("lossy")),
+            WriteablePrefStore::LOSSY_PREF_WRITE_FLAG};
+  }
+
+  // Creates a JsonPrefStore with the given |file_writer|.
+  scoped_refptr<JsonPrefStore> CreatePrefStore(
+      TestImportantFileWriter* file_writer) {
+    base::FilePath file(FILE_PATH_LITERAL("abc.txt"));
+    return new JsonPrefStore(file, file, message_loop_.task_runner(),
+                             scoped_ptr<base::ImportantFileWriter>(file_writer),
+                             scoped_ptr<PrefFilter>());
+  }
+};
+
+TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteBasic) {
+  TestImportantFileWriter* file_writer = new TestImportantFileWriter;
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore(file_writer);
+  PrefDetails normal = GetNormalPrefDetails();
+  PrefDetails lossy = GetLossyPrefDetails();
+
+  // Set a normal pref and check that it gets scheduled to be written.
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->SetValue(normal.key, normal.value->DeepCopy(), normal.flags);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+  file_writer->DoScheduledWrite();
+  ASSERT_EQ("{\"normal\":\"normal\"}", file_writer->GetAndClearLastWrite());
+  ASSERT_FALSE(file_writer->HasPendingWrite());

[Mattias Nissler (ping if slow), 2015/05/07 07:59:53]

Instead of mocking the file writer, you could just spin the message loop using
RunLoop.RunUntilIdle() and check whether the write gets flushed out to disk.

+
+  // Set a lossy pref and check that it is not scheduled to be written.
+  // SetValue/RemoveValue
+  pref_store->SetValue(lossy.key, lossy.value->DeepCopy(), lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->RemoveValue(lossy.key, lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());

[Mattias Nissler (ping if slow), 2015/05/07 07:59:53]

Same here, could just spin the loop and check the result.

+
+  // SetValueSilently/RemoveValueSilently.
+  pref_store->SetValueSilently(lossy.key, lossy.value->DeepCopy(), lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->RemoveValueSilently(lossy.key, lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // ReportValueChanged.
+  pref_store->SetValue(lossy.key, lossy.value->DeepCopy(), lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->ReportValueChanged(lossy.key, lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // Call CommitPendingWrite and check that the lossy pref and the normal pref
+  // are there with the last values set above.
+  pref_store->CommitPendingWrite();
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  ASSERT_EQ("{\"lossy\":\"lossy\",\"normal\":\"normal\"}",
+            file_writer->GetAndClearLastWrite());
+}
+
+TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixed) {
+  TestImportantFileWriter* file_writer = new TestImportantFileWriter;
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore(file_writer);
+  PrefDetails normal = GetNormalPrefDetails();
+  PrefDetails lossy = GetLossyPrefDetails();
+
+  // Set a normal pref and check that it is scheduled to be written.
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->SetValue(normal.key, normal.value->DeepCopy(), normal.flags);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+  // Set a lossy pref and check that the write is still scheduled.
+  pref_store->SetValue(lossy.key, lossy.value->DeepCopy(), lossy.flags);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+  // Call DoScheduledWrite and check both prefs get written.
+  file_writer->DoScheduledWrite();
+  ASSERT_EQ("{\"lossy\":\"lossy\",\"normal\":\"normal\"}",
+            file_writer->GetAndClearLastWrite());
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // Cleanup.
+  pref_store->RemoveValue(normal.key, normal.flags);
+  pref_store->RemoveValue(lossy.key, lossy.flags);
+  pref_store->CommitPendingWrite();
+  file_writer->GetAndClearLastWrite();
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // Set a lossy pref and check that it is not scheduled to be written.
+  pref_store->SetValue(lossy.key, lossy.value->DeepCopy(), lossy.flags);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  // Set a normal pref and check that it is scheduled to be written.
+  pref_store->SetValue(normal.key, normal.value->DeepCopy(), normal.flags);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+  // Call DoScheduledWrite and check both prefs get written.
+  file_writer->DoScheduledWrite();
+  ASSERT_EQ("{\"lossy\":\"lossy\",\"normal\":\"normal\"}",
+            file_writer->GetAndClearLastWrite());
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+}
+
 }  // namespace base