Add synchronous callback support to important_file_writer.cc

components/prefs/json_pref_store.h

 // 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.
 
 #ifndef COMPONENTS_PREFS_JSON_PREF_STORE_H_
 #define COMPONENTS_PREFS_JSON_PREF_STORE_H_
 
 #include <stdint.h>
 
 #include <memory>
@@ skipping 12 matching lines @@
 #include "components/prefs/base_prefs_export.h"
 #include "components/prefs/persistent_pref_store.h"
 
 class PrefFilter;
 
 namespace base {
 class Clock;
 class DictionaryValue;
 class FilePath;
 class HistogramBase;
+class JsonPrefStoreCallbackTest;
 class JsonPrefStoreLossyWriteTest;
 class SequencedTaskRunner;
 class SequencedWorkerPool;
 class Value;
 FORWARD_DECLARE_TEST(JsonPrefStoreTest, WriteCountHistogramTestBasic);
 FORWARD_DECLARE_TEST(JsonPrefStoreTest, WriteCountHistogramTestSinglePeriod);
 FORWARD_DECLARE_TEST(JsonPrefStoreTest, WriteCountHistogramTestMultiplePeriods);
 FORWARD_DECLARE_TEST(JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps);
 }
 
@@ skipping 56 matching lines @@
   PrefReadError ReadPrefs() override;
   void ReadPrefsAsync(ReadErrorDelegate* error_delegate) override;
   void CommitPendingWrite() override;
   void SchedulePendingLossyWrites() override;
   void ReportValueChanged(const std::string& key, uint32_t flags) override;
 
   // Just like RemoveValue(), but doesn't notify observers. Used when doing some
   // 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;
 
  private:
   // Represents a histogram for recording the number of writes to the pref file
   // that occur every kHistogramWriteReportIntervalInMins minutes.
   class COMPONENTS_PREFS_EXPORT WriteCountHistogram {
    public:
     static const int32_t kHistogramWriteReportIntervalMins;
 
@@ skipping 40 matching lines @@
   };
 
   FRIEND_TEST_ALL_PREFIXES(base::JsonPrefStoreTest,
                            WriteCountHistogramTestBasic);
   FRIEND_TEST_ALL_PREFIXES(base::JsonPrefStoreTest,
                            WriteCountHistogramTestSinglePeriod);
   FRIEND_TEST_ALL_PREFIXES(base::JsonPrefStoreTest,
                            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
   // FinalizeFileRead() to that |pref_filter_| which is then responsible for
   // invoking it when done. If there is no |pref_filter_|, FinalizeFileRead()
   // is invoked directly.
   void OnFileRead(std::unique_ptr<ReadResult> read_result);
 
   // ImportantFileWriter::DataSerializer overrides:
   bool SerializeData(std::string* output) override;
@@ skipping 12 matching lines @@
   // Schedule a write with the file writer as long as |flags| doesn't contain
   // WriteablePrefStore::LOSSY_PREF_WRITE_FLAG.
   void ScheduleWrite(uint32_t flags);
 
   const base::FilePath path_;
   const base::FilePath alternate_path_;
   const scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
 
   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.
   base::ImportantFileWriter writer_;
 
   std::unique_ptr<PrefFilter> pref_filter_;
   base::ObserverList<PrefStore::Observer, true> observers_;
 
   std::unique_ptr<ReadErrorDelegate> error_delegate_;
 
   bool initialized_;
   bool filtering_in_progress_;
   bool pending_lossy_write_;
   PrefReadError read_error_;
 
   std::set<std::string> keys_need_empty_value_;
 
   WriteCountHistogram write_count_histogram_;
 
   DISALLOW_COPY_AND_ASSIGN(JsonPrefStore);
 };
 
 #endif  // COMPONENTS_PREFS_JSON_PREF_STORE_H_