Add synchronous callback support to important_file_writer.cc

components/prefs/json_pref_store.h

Index: components/prefs/json_pref_store.h
diff --git a/components/prefs/json_pref_store.h b/components/prefs/json_pref_store.h
index 047fe74abcd88857597e7ac0d398274820de5b7d..43d9ae0353d033d45aa63a67650c99b4d2be1987 100644
--- a/components/prefs/json_pref_store.h
+++ b/components/prefs/json_pref_store.h
@@ -30,6 +30,7 @@ class Clock;
 class DictionaryValue;
 class FilePath;
 class HistogramBase;
+class JsonPrefStoreCallbackTest;
 class JsonPrefStoreLossyWriteTest;
 class SequencedTaskRunner;
 class SequencedWorkerPool;
@@ -106,10 +107,18 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
   // cleanup that shouldn't otherwise alert observers.
   void RemoveValueSilently(const std::string& key, uint32_t flags);
 
-  // Registers |on_next_successful_write| to be called once, on the next
+  // Registers |on_next_successful_write_reply| to be called once, on the next
   // successful write event of |writer_|.
-  void RegisterOnNextSuccessfulWriteCallback(
-      const base::Closure& on_next_successful_write);
+  // |on_next_successful_write_reply| will be called on the thread from which
+  // this method is called and does not need to be thread safe.
+  void RegisterOnNextSuccessfulWriteReply(
+      const base::Closure& on_next_successful_write_reply);
+
+  // Registers |on_next_write_callback| to be called once, on the

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

... to be called once, synchronously, on the ...

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

Done.

+  // next write event of |writer_|.
+  // |on_next_write_callback| must be thread-safe.
+  void RegisterOnNextWriteCallback(

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

RegisterOnNextWriteSynchronousCallback

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

I don't think RegisterOnNextWriteSynchronousCallback is the right name.

1. SynchronousCallback doesn't seem to be the standard way of naming it: there's
no codesearch result for a method signature involving Register, Synchronous and
Callback. 
2. The callback isn't synchronous from the JsonPrefStore's point of view, but
rather from the file writer. 
3. Does it matter to the caller whether it's synchronous or not? As long as
|on_next_write_callback| is thread safe, does it matter on which thread it's
run?

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

Well this use case is very much an edge case, I wouldn't expect this to be done
elsewhere really.
It does matter that it's synchronous, i.e. the very user you're about to
introduce cares because if it's posted anywhere instead of being executed
synchronously after the write then it's not guaranteed to run on shutdown.

Happy to field other naming suggestions, but I think it's core to this API that
it be synchronous (and therefore adding it in the name sounds best to me).

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

Done.

+      const base::Callback<void(bool success)>& on_next_write_callback);
 
   void ClearMutableValues() override;
 
@@ -170,10 +179,20 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
                            WriteCountHistogramTestMultiplePeriods);
   FRIEND_TEST_ALL_PREFIXES(base::JsonPrefStoreTest,
                            WriteCountHistogramTestPeriodWithGaps);
+  friend class base::JsonPrefStoreCallbackTest;
   friend class base::JsonPrefStoreLossyWriteTest;
 
   ~JsonPrefStore() override;
 
+  // If |write_success| is true, runs |on_next_successful_write| on
+  // |reply_task_runner|. Otherwise, re-registers |on_next_successful_write|
+  // on |calling_store|.
+  static void PostWriteCallback(
+      base::WeakPtr<JsonPrefStore> calling_store,
+      const base::Callback<void(bool success)>& on_next_successful_write,
+      scoped_refptr<base::SequencedTaskRunner> reply_task_runner,
+      bool write_success);
+
   // This method is called after the JSON file has been read.  It then hands
   // |value| (or an empty dictionary in some read error cases) to the
   // |pref_filter| if one is set. It also gives a callback pointing at
@@ -206,6 +225,10 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
 
   std::unique_ptr<base::DictionaryValue> prefs_;
 
+  bool has_pending_successful_write_reply_;
+  bool has_pending_write_callback_;
+  base::Closure on_next_successful_write_reply_;
+
   bool read_only_;
 
   // Helper for safely writing pref data.