Add synchronous callback support to important_file_writer.cc

components/prefs/json_pref_store_unittest.cc

Index: components/prefs/json_pref_store_unittest.cc
diff --git a/components/prefs/json_pref_store_unittest.cc b/components/prefs/json_pref_store_unittest.cc
index 32e30ea4506ff27f3a038b9b426543d1a984ec8b..f5fe991766614f704152b5f41e505af9b94a8eba 100644
--- a/components/prefs/json_pref_store_unittest.cc
+++ b/components/prefs/json_pref_store_unittest.cc
@@ -26,6 +26,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/simple_test_clock.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread.h"
 #include "base/values.h"
@@ -958,4 +959,215 @@ TEST_F(JsonPrefStoreLossyWriteTest, ScheduleLossyWrite) {
   ASSERT_EQ("{\"lossy\":\"lossy\"}", GetTestFileContents());
 }
 
-}  // namespace base
+class SuccessfulWriteReplyObserver {
+ public:
+  SuccessfulWriteReplyObserver() : successful_write_reply_observed_(false) {}
+
+  // Returns true if a successful write was observed via on_successful_write()
+  // and resets the observation state to false regardless.
+  bool GetAndResetObservationState() {
+    bool was_successful_write_observed = successful_write_reply_observed_;
+    successful_write_reply_observed_ = false;
+    return was_successful_write_observed;
+  }
+
+  // Register OnWrite() to be called on the next write of |json_pref_store|.
+  void ObserveNextWriteCallback(JsonPrefStore* json_pref_store);
+
+  void OnSuccessfulWrite() {
+    EXPECT_FALSE(successful_write_reply_observed_);
+    successful_write_reply_observed_ = true;
+  }
+
+ private:
+  bool successful_write_reply_observed_;

[gab, 2016/09/19 18:01:30]

Use C++11 member initialization (i.e. = false here) and define default
constructor instead of using initializer lists.

[proberge, 2016/09/19 21:24:26]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteReplyObserver);
+};
+
+void SuccessfulWriteReplyObserver::ObserveNextWriteCallback(
+    JsonPrefStore* json_pref_store) {
+  json_pref_store->RegisterOnNextSuccessfulWriteReply(
+      base::Bind(&SuccessfulWriteReplyObserver::OnSuccessfulWrite,
+                 base::Unretained(this)));
+}
+
+enum WriteCallbackObservationState {
+  NOT_CALLED,
+  CALLED_WITH_ERROR,
+  CALLED_WITH_SUCCESS,
+};
+
+class WriteCallbackObserver {
+ public:
+  WriteCallbackObserver() : observation_state_(NOT_CALLED) {}
+
+  // Register OnWrite() to be called on the next write of |json_pref_store|.
+  void ObserveNextWriteCallback(JsonPrefStore* json_pref_store);
+
+  // Returns true if a write was observed via OnWrite()
+  // and resets the observation state to false regardless.
+  WriteCallbackObservationState GetAndResetObservationState();
+
+  void OnWrite(bool success) {
+    EXPECT_EQ(NOT_CALLED, observation_state_);
+    observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR;
+  }
+
+ private:
+  WriteCallbackObservationState observation_state_;

[gab, 2016/09/19 18:01:31]

C++11 member init here too

[proberge, 2016/09/19 21:24:25]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(WriteCallbackObserver);
+};
+
+void WriteCallbackObserver::ObserveNextWriteCallback(JsonPrefStore* writer) {
+  writer->RegisterOnNextWriteCallback(
+      base::Bind(&WriteCallbackObserver::OnWrite, base::Unretained(this)));
+}
+
+WriteCallbackObservationState
+WriteCallbackObserver::GetAndResetObservationState() {
+  WriteCallbackObservationState state = observation_state_;
+  observation_state_ = NOT_CALLED;
+  return state;
+}
+
+class JsonPrefStoreCallbackTest : public JsonPrefStoreTest {
+ protected:
+  void SetUp() override {
+    JsonPrefStoreTest::SetUp();
+    test_file_ = temp_dir_.path().AppendASCII("test.json");
+  }
+
+  // Creates a JsonPrefStore with the given |file_writer|.

[gab, 2016/09/19 18:01:31]

|file_writer| refers to nothing?

[proberge, 2016/09/19 21:24:26]

Copied the comment from line 827. Removed the comment here and there.

+  scoped_refptr<JsonPrefStore> CreatePrefStore() {
+    return new JsonPrefStore(test_file_, message_loop_.task_runner(),
+                             std::unique_ptr<PrefFilter>());
+  }
+
+  // As above, but with a custom SequencedTaskRunner.
+  scoped_refptr<JsonPrefStore> CreatePrefStore(
+      scoped_refptr<SequencedTaskRunner> task_runner) {

[gab, 2016/09/19 18:01:30]

Unused? Should there be a test with an independent thread using this though?

[proberge, 2016/09/19 21:24:26]

I thought I needed it for TestPostWriteCallbackDuringProfileDeath, but ended up
not using it. 

Any suggestion on what a test with an independent thread would do?

[gab, 2016/09/20 01:27:41]

I guess it wouldn't do much more than re-test ImportantFileWriter. The important
part is that the shutdown test works so what you have here is fine :-)

+    return new JsonPrefStore(test_file_, task_runner,
+                             std::unique_ptr<PrefFilter>());
+  }
+
+  // Return the ImportantFileWriter for a given JsonPrefStore.
+  ImportantFileWriter* GetImportantFileWriter(
+      scoped_refptr<JsonPrefStore> pref_store) {

[gab, 2016/09/19 18:01:30]

scoped_refptr<JsonPrefStore> as a parameter implicitly indicates that the method
may take ownership. When it definitely won't, a raw pointer should be used.

[proberge, 2016/09/19 21:24:26]

Was copied from line 834. Updated there as well.

+    return &(pref_store->writer_);
+  }
+
+  void TriggerFakeWriteForCallback(scoped_refptr<JsonPrefStore> pref_store,

[gab, 2016/09/19 18:01:30]

Same here

[proberge, 2016/09/19 21:24:25]

Done.

+                                   bool success) {
+    JsonPrefStore::PostWriteCallback(
+        pref_store->AsWeakPtr(),
+        base::Bind(&WriteCallbackObserver::OnWrite,
+                   base::Unretained(&write_callback_observer_)),
+        base::SequencedTaskRunnerHandle::Get(), success);
+  }
+
+  SuccessfulWriteReplyObserver successful_write_reply_observer_;
+  WriteCallbackObserver write_callback_observer_;
+
+ private:
+  base::FilePath test_file_;
+};

[gab, 2016/09/19 18:01:30]

DISALLOW_COPY_AND_ASSIGN

[proberge, 2016/09/19 21:24:26]

Looks like DISALLOW_COPY_AND_ASSIGN isn't happy with using
TEST_F(JsonPrefStoreCallbackTest, foo):

c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1067): error
C2512: 'base::JsonPrefStoreCallbackTest': no appropriate default constructor
available
c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1033): note:
see declaration of 'base::JsonPrefStoreCallbackTest'

[dcheng, 2016/09/20 01:08:57]

You'll have to give the test an explicit default constructor in that case, e.g.
JsonPrefStoreCallbackTest() { }

[gab, 2016/09/20 01:27:41]

This is because DISALLOW_COPY_AND_ASSIGN is implemented by making the copy
constructor private. This has the side-effect of deleting the implicit default
constructor. As such you need to explicitly add

JsonPrefStoreCallbackTest() = default;

at the top of this class' public section.


Also note: in the old Chromium days, it was common for people not to put
DISALLOW_COPY_AND_ASSIGN in test fixtures, the rules have since been clarified
to enforce it everywhere (that's why you'll see old code that doesn't do this
still today but be told to do it in new code that merely mimics it)

[proberge, 2016/09/20 15:15:52]

Done.

+
+TEST_F(JsonPrefStoreCallbackTest, TestPostWriteCallback) {
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
+  ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store);
+
+  // Test RegisterOnNextWriteCallback after RegisterOnNextSuccessfulWriteReply.
+  successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get());
+  write_callback_observer_.ObserveNextWriteCallback(pref_store.get());
+  file_writer->WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_TRUE(write_callback_observer_.GetAndResetObservationState());
+
+  // Test RegisterOnNextSuccessfulWriteReply after RegisterOnNextWriteCallback.
+  successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get());
+  write_callback_observer_.ObserveNextWriteCallback(pref_store.get());
+  file_writer->WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_TRUE(write_callback_observer_.GetAndResetObservationState());
+
+  // Test RegisterOnNextSuccessfulWriteReply only.
+  successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get());
+  file_writer->WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(write_callback_observer_.GetAndResetObservationState());
+
+  // Test RegisterOnNextWriteCallback only.
+  write_callback_observer_.ObserveNextWriteCallback(pref_store.get());
+  file_writer->WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_TRUE(write_callback_observer_.GetAndResetObservationState());
+}
+
+TEST_F(JsonPrefStoreCallbackTest, TestPostWriteCallbackWithFakeFailure) {
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
+
+  // Confirm that the observers are invoked.
+  successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get());
+  TriggerFakeWriteForCallback(pref_store, true);
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+
+  // Confirm that the observation states were reset.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+
+  // Confirm that re-installing the observers works for another write.
+  successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get());
+  TriggerFakeWriteForCallback(pref_store, true);
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+
+  // Confirm that the successful observer is not invoked by an unsuccessful
+  // write, and that the synchronous observer is invoked.
+  successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get());
+  TriggerFakeWriteForCallback(pref_store, false);
+  RunLoop().RunUntilIdle();
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_ERROR,
+            write_callback_observer_.GetAndResetObservationState());
+
+  // Do a real write, and confirm that the successful observer was invoked after
+  // being set by |PostWriteCallback| by the last TriggerFakeWriteCallback.
+  ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store);
+  file_writer->WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+}
+
+TEST_F(JsonPrefStoreCallbackTest, TestPostWriteCallbackDuringProfileDeath) {
+  // Create a JsonPrefStore and attach observers to it, then delete it by making
+  // it go out of scope to simulate profile switch or Chrome shutdown.
+  {
+    scoped_refptr<JsonPrefStore> soon_out_of_scope_pref_store =
+        CreatePrefStore();
+    ImportantFileWriter* file_writer =
+        GetImportantFileWriter(soon_out_of_scope_pref_store);
+    successful_write_reply_observer_.ObserveNextWriteCallback(
+        soon_out_of_scope_pref_store.get());
+    write_callback_observer_.ObserveNextWriteCallback(
+        soon_out_of_scope_pref_store.get());
+    file_writer->WriteNow(WrapUnique(new std::string("foo")));
+  }
+  RunLoop().RunUntilIdle();
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+}
+
+}  // namespace base