Introduce a new framework for back-and-forth tracked/protected preferences migration.

base/prefs/json_pref_store.h

Index: base/prefs/json_pref_store.h
diff --git a/base/prefs/json_pref_store.h b/base/prefs/json_pref_store.h
index ad13feba3f6e1279c7c2bee4d597df35d1698842..91139b0b2ec031d40c1f33ad5a5382a67976fc2f 100644
--- a/base/prefs/json_pref_store.h
+++ b/base/prefs/json_pref_store.h
@@ -9,10 +9,12 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/important_file_writer.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/observer_list.h"
 #include "base/prefs/base_prefs_export.h"
@@ -34,6 +36,27 @@ class BASE_PREFS_EXPORT JsonPrefStore
     : public PersistentPrefStore,
       public base::ImportantFileWriter::DataSerializer {
  public:
+  // A callback to be invoked when |prefs| have been read (and possibly
+  // pre-modified) and are now ready to be handed back to this callback's
+  // builder. |schedule_write| indicates whether a write should be immediately
+  // scheduled (typically because the OnFileReadInterceptor has already altered
+  // the |prefs|).
+  // Returns a weak pointer to this JsonPrefStore which can be used by the
+  // caller for future cleanup operations while this JsonPrefStore is still up.
+  typedef base::Callback<base::WeakPtr<JsonPrefStore>(
+      scoped_ptr<base::DictionaryValue> prefs,
+      bool schedule_write)> FinalizePrefsReadCallback;
+
+  // A callback to be invoked from OnFileRead, after the read errors have been
+  // handled, but before |prefs| is handed to this JsonPrefStore. The callee of
+  // this callback is responsible for invoking |finalize_prefs_read| to hand
+  // |prefs| back to this JsonPrefStore when it's done -- this JsonPrefStore
+  // will not consider its read complete nor invoke observers until it has been
+  // invoked.
+  typedef base::Callback<void(scoped_ptr<base::DictionaryValue> prefs,
+                              const FinalizePrefsReadCallback&
+                                  finalize_prefs_read)> OnFileReadInterceptor;
+
   // Returns instance of SequencedTaskRunner which guarantees that file
   // operations on the same file will be executed in sequenced order.
   static scoped_refptr<base::SequencedTaskRunner> GetTaskRunnerForFile(
@@ -68,11 +91,35 @@ class BASE_PREFS_EXPORT JsonPrefStore
   virtual void CommitPendingWrite() OVERRIDE;
   virtual void ReportValueChanged(const std::string& key) OVERRIDE;
 
-  // This method is called after JSON file has been read. Method takes
-  // ownership of the |value| pointer. Note, this method is used with
-  // asynchronous file reading, so class exposes it only for the internal needs.
-  // (read: do not call it manually).
-  void OnFileRead(base::Value* value_owned, PrefReadError error, bool no_dir);
+  // 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);
+
+  // Registers |on_next_successful_write| to be called once, on the next
+  // successful write event of |writer_|.
+  void RegisterOnNextSuccessfulWriteCallback(
+      const base::Closure& on_next_successful_write);
+
+  // Registers |on_file_read_interceptor| to intercept the next OnFileRead
+  // event. At most one OnFileReadInterceptor should be registered per file
+  // read.
+  void InterceptNextFileRead(
+      const OnFileReadInterceptor& on_file_read_interceptor);
+
+  // 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
+  // |on_file_read_interceptor_| if one is set. It also gives a callback
+  // pointing at FinalizeFileRead() to the |on_file_read_interceptor_| which is
+  // then responsible for invoking it when done. If there is no
+  // |on_file_read_interceptor_|, FinalizeFileRead() is invoked directly.
+  // Note, this method is used with asynchronous file reading, so this class
+  // exposes it only for the internal needs (read: do not call it manually).
+  // TODO(gab): Move this method to the private section and hand a callback to
+  // it to FileThreadDeserializer rather than exposing this public method and
+  // giving a JsonPrefStore* to FileThreadDeserializer.
+  void OnFileRead(scoped_ptr<base::Value> value,
+                  PrefReadError error,
+                  bool no_dir);
 
  private:
   virtual ~JsonPrefStore();
@@ -80,6 +127,29 @@ class BASE_PREFS_EXPORT JsonPrefStore
   // ImportantFileWriter::DataSerializer overrides:
   virtual bool SerializeData(std::string* output) OVERRIDE;
 
+  // This method is called after the JSON file has been read and the result has
+  // potentially been intercepted and modified by |on_file_read_interceptor_|.
+  // |initialization_successful| is pre-determined by OnFileRead() and should
+  // be used when reporting OnInitializationCompleted().
+  // |schedule_write| indicates whether a write should be immediately scheduled
+  // (typically because the OnFileReadInterceptor has already altered the
+  // |prefs|) -- this will be ignored if this store is read-only.
+  // Returns a weak pointer to this JsonPrefStore which can be used for future
+  // cleanup operations while this JsonPrefStore is still up.
+  base::WeakPtr<JsonPrefStore> FinalizeFileRead(
+      bool initialization_successful,
+      scoped_ptr<base::DictionaryValue> prefs,
+      bool schedule_write);
+
+  // Callback to be invoked only once (and subsequently reset) on the next
+  // OnFileRead event. It will be allowed to modify the JSON value read from
+  // disk before this JsonPrefStore gets to declare itself initialized and
+  // begins using it. It is responsible for handing it back to this
+  // JsonPrefStore via the FinalizePrefReadCallback handed to it.
+  // Note: Setting an |on_file_read_interceptor_| may make ReadPrefs()
+  // asynchronous if the |on_file_read_interceptor_| is asynchronous.
+  OnFileReadInterceptor on_file_read_interceptor_;
+
   base::FilePath path_;
   const scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
 
@@ -100,6 +170,8 @@ class BASE_PREFS_EXPORT JsonPrefStore
 
   std::set<std::string> keys_need_empty_value_;
 
+  base::WeakPtrFactory<JsonPrefStore> weak_factory_;

[Bernhard Bauer, 2014/04/30 09:31:05]

You now have a WeakPtrFactory in a refcounted object. That, as well as the fact
that you are wielding weak pointers to this class outside of it should be a red
flag. See also below...

[gab, 2014/04/30 14:32:53]

I don't see why this is a red flag. Ref-counting is an ownership model; weak
pointers are a reference model. I don't see why the two can't be used together.

As weak_ptr.h says:
// Weak pointers are useful when an object needs to be accessed safely by one
// or more objects other than its owner, and those callers can cope with the
// object vanishing and e.g. tasks posted to it being silently dropped.
// Reference-counting such an object would complicate the ownership graph and
// make it harder to reason about the object's lifetime.

(this comment only mentions that it's better to use WeakPtr than RefCounting
whenever possible, but I'd argue it's simlarly better to use WeakPtrs for
already ref-counted objects than handing more references for similar reasons).

[Bernhard Bauer, 2014/05/01 12:37:09]

Hm, I read that comment more as, "if you have an object and want to safely
access it at a later point, prefer using weak pointers than starting to refcount
it". But anyway, what I meant by red flag is not that it's a no-go, just that
you should take a close look at what you're doing. I have so far never
encountered a situation where refcounting + weak pointers turned out to be the
best solution.

+
   DISALLOW_COPY_AND_ASSIGN(JsonPrefStore);
 };