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 43e051ebcfadf108e3a244420c5a01d5397396a9..15662ca818ea7c27184e09eb579a1baa7e79c17a 100644
--- a/base/files/important_file_writer_unittest.cc
+++ b/base/files/important_file_writer_unittest.cc
@@ -71,7 +71,7 @@ class SuccessfulWriteObserver {
 
 void SuccessfulWriteObserver::ObserveNextSuccessfulWrite(
     ImportantFileWriter* writer) {
-  writer->RegisterOnNextSuccessfulWriteCallback(base::Bind(
+  writer->RegisterOnNextSuccessfulWriteReply(base::Bind(
       &SuccessfulWriteObserver::on_successful_write, base::Unretained(this)));
 }
 
@@ -81,71 +81,189 @@ bool SuccessfulWriteObserver::GetAndResetObservationState() {
   return was_successful_write_observed;
 }
 
+enum WriteCallbackObservationState {
+  NOT_CALLED,
+  CALLED_WITH_ERROR,
+  CALLED_WITH_SUCCESS,
+};
+
+class SynchronousWriteCallbackObserver {
+ public:
+  SynchronousWriteCallbackObserver() : observation_state_(NOT_CALLED) {}
+
+  // Register on_write() to be called on the next write of |writer|.
+  void ObserveNextWriteCallback(ImportantFileWriter* writer);
+
+  // Returns true if a write was observed via on_write()
+  // and resets the observation state to false regardless.
+  WriteCallbackObservationState GetAndResetObservationState();
+
+ private:
+  void on_write(bool success) {
+    EXPECT_EQ(NOT_CALLED, observation_state_);
+    observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR;
+  }
+
+  WriteCallbackObservationState observation_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(SynchronousWriteCallbackObserver);
+};
+
+void SynchronousWriteCallbackObserver::ObserveNextWriteCallback(
+    ImportantFileWriter* writer) {
+  writer->RegisterOnNextWriteSynchronousCallback(base::Bind(
+      &SynchronousWriteCallbackObserver::on_write, base::Unretained(this)));
+}
+
+WriteCallbackObservationState
+SynchronousWriteCallbackObserver::GetAndResetObservationState() {
+  WriteCallbackObservationState state = observation_state_;
+  observation_state_ = NOT_CALLED;
+  return state;
+}
+
 }  // namespace
 
 class ImportantFileWriterTest : public testing::Test {
  public:
-  ImportantFileWriterTest() { }
+  ImportantFileWriterTest()
+      : io_thread_("ImportantFileWriter test IO thread"),
+        wait_helper_(base::WaitableEvent::ResetPolicy::MANUAL,
+                     base::WaitableEvent::InitialState::NOT_SIGNALED) {
+    io_thread_.StartWithOptions(
+        base::Thread::Options(base::MessageLoop::TYPE_IO, 0));

[gab, 2016/09/07 19:40:52]

I don't think you specifically need an IO thread ("TYPE_IO" is different than
and not required for "file I/O")

For the same reason, don't call it "io_thread_", something like
"file_writer_thread_" will be more correct (same for its thread name).

[proberge, 2016/09/08 17:19:31]

Done.

+  }
+
+  ~ImportantFileWriterTest() { io_thread_.Stop(); }

[gab, 2016/09/07 19:40:52]

Stop() is implicit in base::Thread's destructor, don't need to invoke it here.

[proberge, 2016/09/08 17:19:31]

Done.

+
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     file_ = temp_dir_.path().AppendASCII("test-file");
   }
 
+  void WaitForWriteTask() {
+    wait_helper_.Reset();
+    io_thread_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(&base::WaitableEvent::Signal,
+                              base::Unretained(&wait_helper_)));
+    wait_helper_.Wait();
+  }
+
+  scoped_refptr<SingleThreadTaskRunner> GetTestThreadTaskRunner() {
+    return io_thread_.task_runner();
+  }
+
  protected:
-  SuccessfulWriteObserver successful_write_observer_;
+  SuccessfulWriteObserver successful_write_reply_observer_;
+  SynchronousWriteCallbackObserver write_callback_observer_;
   FilePath file_;
   MessageLoop loop_;
 
  private:
   ScopedTempDir temp_dir_;
+
+  // Used for fine-grained control of thread execution.
+  base::Thread io_thread_;
+  base::WaitableEvent wait_helper_;
 };
 
 TEST_F(ImportantFileWriterTest, Basic) {
   ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   writer.WriteNow(WrapUnique(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(successful_write_reply_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) {
-  ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
+TEST_F(ImportantFileWriterTest, BasicWithWriteObservers) {
+  ImportantFileWriter writer(file_, GetTestThreadTaskRunner());

[gab, 2016/09/07 19:40:52]

Since it's only for this test actually, bring the base::Thread as a local
variable here instead of adding it to the test fixture.

[proberge, 2016/09/08 17:19:31]

Done. I thought it would be useful to have it in the test fixture to avoid the
duplicated logic.

[gab, 2016/09/08 17:50:46]

Right, except that it then started an unnecessary thread in every other test
using the fixture. Once Reset() is removed per other comment, it's not that much
duplication really and the test is more readable because the logic is local.

+
   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  successful_write_reply_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(WrapUnique(new std::string("foo")));
+
+  // Make sure tasks posted by WriteNow() have ran before continuing.
+  WaitForWriteTask();
+
+  // The |successful_write_reply_observer_| should not yet have been
+  // notified yet but the |write_callback_observer_| should have been.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+
+  // There should be a pending task to notify |successful_write_reply_observer_|
+  // in this message loop.
   RunLoop().RunUntilIdle();
 
   // Confirm that the observer is invoked.
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_TRUE(successful_write_reply_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);
+  // Confirm that re-installing the observers works for another write.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  successful_write_reply_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(WrapUnique(new std::string("bar")));
+
+  WaitForWriteTask();
+
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+
   RunLoop().RunUntilIdle();
 
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("bar", GetFileContent(writer.path()));
 
-  // Confirm that writing again without re-installing the observer doesn't
+  // Confirm that writing again without re-installing the observers doesn't
   // result in a notification.
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   writer.WriteNow(WrapUnique(new std::string("baz")));
+
+  WaitForWriteTask();
   RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
 
+TEST_F(ImportantFileWriterTest, FailedWriteWithWriteObservers) {
+  // 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_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  successful_write_reply_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+
+  // Confirm that the successful write observer was not invoked, and that the
+  // write observer was invoked with its boolean parameter set to false.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_ERROR,
+            write_callback_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(PathExists(writer.path()));
+}
+
 TEST_F(ImportantFileWriterTest, ScheduleWrite) {
   ImportantFileWriter writer(file_,
                              ThreadTaskRunnerHandle::Get(),