Add synchronous callback support to important_file_writer.cc

base/files/important_file_writer_unittest.cc

Index: base/files/important_file_writer_unittest.cc
diff --git a/base/files/important_file_writer_unittest.cc b/base/files/important_file_writer_unittest.cc
index 3fd71e75949a2b756344753b23347b189a2b94a4..125e14b92f1ff492248bd8f4c8843b000cca938b 100644
--- a/base/files/important_file_writer_unittest.cc
+++ b/base/files/important_file_writer_unittest.cc
@@ -46,39 +46,45 @@ class DataSerializer : public ImportantFileWriter::DataSerializer {
   const std::string data_;
 };
 
-class SuccessfulWriteObserver {
+enum WriteCallbackObservationState {
+  NOT_CALLED,
+  CALLED_WITH_ERROR,
+  CALLED_WITH_SUCCESS,
+};
+
+class WriteCallbackObserver {
  public:
-  SuccessfulWriteObserver() : successful_write_observed_(false) {}
+  WriteCallbackObserver() : observation_state_(NOT_CALLED) {}
 
-  // Register on_successful_write() to be called on the next successful write
-  // of |writer|.
-  void ObserveNextSuccessfulWrite(ImportantFileWriter* writer);
+  // Register OnWrite() to be called on the next write of |writer|.
+  void ObserveNextWriteCallback(ImportantFileWriter* writer);
 
-  // Returns true if a successful write was observed via on_successful_write()
+  // Returns true if a write was observed via OnWrite()
   // and resets the observation state to false regardless.
-  bool GetAndResetObservationState();
+  WriteCallbackObservationState GetAndResetObservationState();
 
  private:
-  void on_successful_write() {
-    EXPECT_FALSE(successful_write_observed_);
-    successful_write_observed_ = true;
+  void OnWrite(bool success) {
+    EXPECT_EQ(NOT_CALLED, observation_state_);
+    observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR;
   }
 
-  bool successful_write_observed_;
+  WriteCallbackObservationState observation_state_;
 
-  DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteObserver);
+  DISALLOW_COPY_AND_ASSIGN(WriteCallbackObserver);
 };
 
-void SuccessfulWriteObserver::ObserveNextSuccessfulWrite(
+void WriteCallbackObserver::ObserveNextWriteCallback(
     ImportantFileWriter* writer) {
-  writer->RegisterOnNextSuccessfulWriteCallback(base::Bind(
-      &SuccessfulWriteObserver::on_successful_write, base::Unretained(this)));
+  writer->RegisterOnNextWriteCallback(
+      base::Bind(&WriteCallbackObserver::OnWrite, base::Unretained(this)));
 }
 
-bool SuccessfulWriteObserver::GetAndResetObservationState() {
-  bool was_successful_write_observed = successful_write_observed_;
-  successful_write_observed_ = false;
-  return was_successful_write_observed;
+WriteCallbackObservationState
+WriteCallbackObserver::GetAndResetObservationState() {
+  WriteCallbackObservationState state = observation_state_;
+  observation_state_ = NOT_CALLED;
+  return state;
 }
 
 }  // namespace
@@ -92,7 +98,7 @@ class ImportantFileWriterTest : public testing::Test {
   }
 
  protected:
-  SuccessfulWriteObserver successful_write_observer_;
+  WriteCallbackObserver write_callback_observer_;
   FilePath file_;
   MessageLoop loop_;
 
@@ -103,49 +109,71 @@ class ImportantFileWriterTest : public testing::Test {
 TEST_F(ImportantFileWriterTest, Basic) {
   ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  writer.WriteNow(MakeUnique<std::string>("foo"));
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  writer.WriteNow(WrapUnique(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
 }
 
-TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
+TEST_F(ImportantFileWriterTest, WriteWithObserver) {
   ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
+

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

rm this empty line to be consistent with surrounding tests

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

Done.

   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
-  writer.WriteNow(MakeUnique<std::string>("foo"));
-  RunLoop().RunUntilIdle();
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
 
   // Confirm that the observer is invoked.
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("foo")));

[dcheng, 2016/09/16 22:42:05]

Nit: MakeUnique<std::string> here and elsewhere.

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

Done.

+  RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
 
   // Confirm that re-installing the observer works for another write.
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
-  writer.WriteNow(MakeUnique<std::string>("bar"));
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("bar")));
   RunLoop().RunUntilIdle();
 
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("bar", GetFileContent(writer.path()));
 
   // Confirm that writing again without re-installing the observer doesn't
   // result in a notification.
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  writer.WriteNow(MakeUnique<std::string>("baz"));
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  writer.WriteNow(WrapUnique(new std::string("baz")));
   RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
 
+TEST_F(ImportantFileWriterTest, FailedWriteWithObserver) {
+  // Use an invalid file path (relative paths are invalid) to get a
+  // FILE_ERROR_ACCESS_DENIED error when trying to write the file.
+  ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"),
+                             ThreadTaskRunnerHandle::Get());
+  EXPECT_FALSE(PathExists(writer.path()));
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+
+  // Confirm that the write observer was invoked with its boolean parameter set
+  // to false.
+  EXPECT_EQ(CALLED_WITH_ERROR,
+            write_callback_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(PathExists(writer.path()));
+}
+

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

Re-add the test with the independent thread and confirm that the callback is
invoked on that thread, not on the main thread (by confirming that running the
current loop doesn't result in the notification but flushing the independent
thread does -- you'll need make this deterministic by posting a
WaitableEvent::Wait task to the thread before calling WriteNow() so that the
callback is queued behind it and you control when it should be released,
otherwise the callback could still racily run while you're verifying that it
doesn't run as part of the main loop).

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

Done. Did not add a WaitableEvent::Wait task as I don't think this is
non-deterministic as implemented. Please take a look.

 TEST_F(ImportantFileWriterTest, ScheduleWrite) {
   ImportantFileWriter writer(file_,
                              ThreadTaskRunnerHandle::Get(),