Add synchronous callback support to important_file_writer.cc

components/prefs/json_pref_store.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 "components/prefs/json_pref_store.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/task_runner_util.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/default_clock.h"
 #include "base/values.h"
 #include "components/prefs/pref_filter.h"
 
 // Result returned from internal read tasks.
 struct JsonPrefStore::ReadResult {
  public:
   ReadResult();
   ~ReadResult();
@@ skipping 128 matching lines @@
     : path_(pref_filename),
       alternate_path_(pref_alternate_filename),
       sequenced_task_runner_(sequenced_task_runner),
       prefs_(new base::DictionaryValue()),
       read_only_(false),
       writer_(pref_filename, sequenced_task_runner),
       pref_filter_(std::move(pref_filter)),
       initialized_(false),
       filtering_in_progress_(false),
       pending_lossy_write_(false),
+      has_pending_successful_write_reply_(false),
+      has_pending_write_callback_(false),
       read_error_(PREF_READ_ERROR_NONE),
       write_count_histogram_(writer_.commit_interval(), path_) {
   DCHECK(!path_.empty());
 }
 
 bool JsonPrefStore::GetValue(const std::string& key,
                              const base::Value** result) const {
   DCHECK(CalledOnValidThread());
 
   base::Value* tmp = nullptr;
@@ skipping 133 matching lines @@
   DCHECK(CalledOnValidThread());
 
   if (pref_filter_)
     pref_filter_->FilterUpdate(key);
 
   FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
 
   ScheduleWrite(flags);
 }
 
-void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
-    const base::Closure& on_next_successful_write) {
-  DCHECK(CalledOnValidThread());
+// static
+void JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback(
+    base::WeakPtr<JsonPrefStore> calling_store,
+    bool write_success) {
+  if (!calling_store)
+    return;
 
-  writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
+  DCHECK(calling_store->CalledOnValidThread());
+
+  calling_store->has_pending_write_callback_ = false;
+  if (calling_store->has_pending_successful_write_reply_) {
+    calling_store->has_pending_successful_write_reply_ = false;
+    if (write_success) {
+      calling_store->on_next_successful_write_reply_.Run();
+    } else {
+      calling_store->RegisterOnNextSuccessfulWriteReply(
+          calling_store->on_next_successful_write_reply_);
+    }
+  }
+}
+
+// static
+void JsonPrefStore::PostWriteCallback(
+    base::WeakPtr<JsonPrefStore> calling_store,

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

Since this method should be using member state, it feels scary to take this as a
parameter (even though it's only being forward in practice which is okay).

Instead take a
const base::Callback<void(bool success)>& on_next_write_reply,
then the WeakPtr can be bound into it ahead of time and hidden from this static
method entirely (Bind supports partial binding).

(and probably s/on_next_write/on_next_write_callback/ ?

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

Done.

+    const base::Callback<void(bool success)>& on_next_write,
+    scoped_refptr<base::SequencedTaskRunner> reply_task_runner,
+    bool write_success) {
+  if (!on_next_write.is_null()) {
+    on_next_write.Run(write_success);
+  }
+
+  // We can't use |calling_store| on the current thread. Bounce back to the
+  // |reply_task_runner| which is the correct sequenced thread.
+  reply_task_runner->PostTask(
+      FROM_HERE, base::Bind(&RunOrScheduleNextSuccessfulWriteCallback,
+                            calling_store, write_success));
+}
+
+void JsonPrefStore::RegisterOnNextSuccessfulWriteReply(
+    const base::Closure& on_next_successful_write_reply) {

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

DCHECK(CalledOnValidThread());

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

Done.

+  DCHECK(!has_pending_successful_write_reply_);
+
+  has_pending_successful_write_reply_ = true;
+  on_next_successful_write_reply_ = on_next_successful_write_reply;
+
+  if (!has_pending_write_callback_) {

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

Add a meta comment here (or elsewhere but this seems like a good place) on how
this whole dance is performed (it does work, but it took me some time to
understand why it does).

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

Done.

+    writer_.RegisterOnNextWriteCallback(
+        base::Bind(&PostWriteCallback, this->AsWeakPtr(),
+                   base::Callback<void(bool success)>(),
+                   base::SequencedTaskRunnerHandle::Get()));
+  }
+}
+
+void JsonPrefStore::RegisterOnNextWriteCallback(
+    const base::Callback<void(bool success)>& on_next_write_callback) {

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

DCHECK(CalledOnValidThread());

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

Done.

+  DCHECK(!has_pending_write_callback_);
+
+  has_pending_write_callback_ = true;
+
+  writer_.RegisterOnNextWriteCallback(
+      base::Bind(&PostWriteCallback, this->AsWeakPtr(), on_next_write_callback,

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

AsWeakPtr()

(no need to explicitly add "this->")

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

Done.

+                 base::SequencedTaskRunnerHandle::Get()));
 }
 
 void JsonPrefStore::ClearMutableValues() {
   NOTIMPLEMENTED();
 }
 
 void JsonPrefStore::OnFileRead(std::unique_ptr<ReadResult> read_result) {
   DCHECK(CalledOnValidThread());
 
   DCHECK(read_result);
@@ skipping 192 matching lines @@
   DCHECK_EQ(31, num_buckets);
 
   // The histogram below is an expansion of the UMA_HISTOGRAM_CUSTOM_COUNTS
   // macro adapted to allow for a dynamically suffixed histogram name.
   // Note: The factory creates and owns the histogram.
   base::HistogramBase* histogram = base::Histogram::FactoryGet(
       histogram_name, min_value, max_value, num_buckets,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   return histogram;
 }