Add synchronous callback support to important_file_writer.cc

base/files/important_file_writer_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/important_file_writer.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ skipping 28 matching lines @@
 
   bool SerializeData(std::string* output) override {
     output->assign(data_);
     return true;
   }
 
  private:
   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
 
 class ImportantFileWriterTest : public testing::Test {
  public:
   ImportantFileWriterTest() { }
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     file_ = temp_dir_.GetPath().AppendASCII("test-file");
   }
 
  protected:
-  SuccessfulWriteObserver successful_write_observer_;
+  WriteCallbackObserver write_callback_observer_;
   FilePath file_;
   MessageLoop loop_;
 
  private:
   ScopedTempDir temp_dir_;
 };
 
 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"));
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+
+  // Confirm that the observer is invoked.
+  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();
 
-  // 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);
-  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(),
                              TimeDelta::FromMilliseconds(25));
   EXPECT_FALSE(writer.HasPendingWrite());
   DataSerializer serializer("foo");
   writer.ScheduleWrite(&serializer);
   EXPECT_TRUE(writer.HasPendingWrite());
   ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, MessageLoop::QuitWhenIdleClosure(),
@@ skipping 30 matching lines @@
   writer.ScheduleWrite(&baz);
   ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, MessageLoop::QuitWhenIdleClosure(),
       TimeDelta::FromMilliseconds(100));
   RunLoop().Run();
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
 
 }  // namespace base