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..b8920a07679e890454eb0d399547cf891aa60866 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,91 @@ 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());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   writer.WriteNow(MakeUnique<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());
   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+
+  // Confirm that the observer is invoked.
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(MakeUnique<std::string>("foo"));
   RunLoop().RunUntilIdle();
 
-  // Confirm that the observer is invoked.
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  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);
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(MakeUnique<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());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   writer.WriteNow(MakeUnique<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(MakeUnique<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()));
+}
+
+TEST_F(ImportantFileWriterTest, CallbackRunsOnWriterThread) {
+  base::Thread file_writer_thread("ImportantFileWriter test thread");
+  file_writer_thread.Start();
+  ImportantFileWriter writer(file_, file_writer_thread.task_runner());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(MakeUnique<std::string>("foo"));
+  RunLoop().RunUntilIdle();
+
+  // Expect the callback to not have been executed before the write.
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());

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

This is racy and you will need a WaitableEvent::Wait task queued before calling
WriteNow(). Otherwise tasks posted by WriteNow() can execute on the other thread
at any point between WriteNow() and the end of FlushForTesting().

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

I don't see it: WriteNow posts a task on the file_writer_thread. If we don't run
FlushForTesting, the callback hasn't been posted to any task runners.

+
+  // Make sure tasks posted by WriteNow() have ran before continuing.
+  file_writer_thread.FlushForTesting();
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("foo", GetFileContent(writer.path()));
+}
+
 TEST_F(ImportantFileWriterTest, ScheduleWrite) {
   ImportantFileWriter writer(file_,
                              ThreadTaskRunnerHandle::Get(),