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

base/prefs/json_pref_store.cc

Index: base/prefs/json_pref_store.cc
diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc
index b0a16cc11431e01fd8661949388c11cdc972f156..8dc46fedfaf3ee7b925d197e0b7cc845ab787705 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -59,7 +59,7 @@ class FileThreadDeserializer
   // Reports deserialization result on the origin thread.
   void ReportOnOriginThread() {
     DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
-    delegate_->OnFileRead(value_.release(), error_, no_dir_);
+    delegate_->OnFileRead(value_.Pass(), error_, no_dir_);
   }
 
   static base::Value* DoReading(const base::FilePath& path,
@@ -234,15 +234,16 @@ PersistentPrefStore::PrefReadError JsonPrefStore::GetReadError() const {
 
 PersistentPrefStore::PrefReadError JsonPrefStore::ReadPrefs() {
   if (path_.empty()) {
-    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(
+        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return PREF_READ_ERROR_FILE_NOT_SPECIFIED;
   }
 
   PrefReadError error;
   bool no_dir;
-  base::Value* value =
-      FileThreadDeserializer::DoReading(path_, &error, &no_dir);
-  OnFileRead(value, error, no_dir);
+  scoped_ptr<base::Value> value(
+      FileThreadDeserializer::DoReading(path_, &error, &no_dir));
+  OnFileRead(value.Pass(), error, no_dir);
   return error;
 }
 
@@ -250,7 +251,8 @@ void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate *error_delegate) {
   initialized_ = false;
   error_delegate_.reset(error_delegate);
   if (path_.empty()) {
-    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(
+        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return;
   }
 
@@ -276,53 +278,60 @@ void JsonPrefStore::ReportValueChanged(const std::string& key) {
     writer_.ScheduleWrite(this);
 }
 
-void JsonPrefStore::OnFileRead(base::Value* value_owned,
+void JsonPrefStore::InterceptNextFileRead(
+    const OnFileReadInterceptor& on_file_read_interceptor) {
+  on_file_read_interceptor_.reset(
+      new OnFileReadInterceptor(on_file_read_interceptor));
+}
+
+void JsonPrefStore::OnFileRead(scoped_ptr<base::Value> value,
                                PersistentPrefStore::PrefReadError error,
                                bool no_dir) {
-  scoped_ptr<base::Value> value(value_owned);
-  read_error_ = error;
+  scoped_ptr<base::DictionaryValue> unprocessed_prefs(
+      new base::DictionaryValue);
 
-  if (no_dir) {
-    FOR_EACH_OBSERVER(PrefStore::Observer,
-                      observers_,
-                      OnInitializationCompleted(false));
-    return;
-  }
+  read_error_ = error;
 
-  initialized_ = true;
+  bool initialization_successful = !no_dir;
 
-  switch (error) {
-    case PREF_READ_ERROR_ACCESS_DENIED:
-    case PREF_READ_ERROR_FILE_OTHER:
-    case PREF_READ_ERROR_FILE_LOCKED:
-    case PREF_READ_ERROR_JSON_TYPE:
-    case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
-      read_only_ = true;
-      break;
-    case PREF_READ_ERROR_NONE:
-      DCHECK(value.get());
-      prefs_.reset(static_cast<base::DictionaryValue*>(value.release()));
-      break;
-    case PREF_READ_ERROR_NO_FILE:
-      // If the file just doesn't exist, maybe this is first run.  In any case
-      // there's no harm in writing out default prefs in this case.
-      break;
-    case PREF_READ_ERROR_JSON_PARSE:
-    case PREF_READ_ERROR_JSON_REPEAT:
-      break;
-    default:
-      NOTREACHED() << "Unknown error: " << error;
+  if (initialization_successful) {
+    switch (error) {
+      case PREF_READ_ERROR_ACCESS_DENIED:
+      case PREF_READ_ERROR_FILE_OTHER:
+      case PREF_READ_ERROR_FILE_LOCKED:
+      case PREF_READ_ERROR_JSON_TYPE:
+      case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
+        read_only_ = true;
+        break;
+      case PREF_READ_ERROR_NONE:
+        DCHECK(value.get());
+        unprocessed_prefs.reset(
+            static_cast<base::DictionaryValue*>(value.release()));
+        break;
+      case PREF_READ_ERROR_NO_FILE:
+        // If the file just doesn't exist, maybe this is first run.  In any case
+        // there's no harm in writing out default prefs in this case.
+        break;
+      case PREF_READ_ERROR_JSON_PARSE:
+      case PREF_READ_ERROR_JSON_REPEAT:
+        break;
+      default:

[Bernhard Bauer, 2014/04/29 15:13:04]

Could we get rid of the default statement?

[gab, 2014/04/29 15:46:44]

Yes, if we provide a case for PREF_READ_ERROR_MAX_ENUM; which is kind of ugly,
but better than a default IMO as it will at least ensure that anyone adding to
this enum gets an error when compiling this switch statement in clang.

Done.

+        NOTREACHED() << "Unknown error: " << error;
+    }
   }
 
-  if (pref_filter_ && pref_filter_->FilterOnLoad(prefs_.get()))
-    writer_.ScheduleWrite(this);
-
-  if (error_delegate_.get() && error != PREF_READ_ERROR_NONE)
-    error_delegate_->OnError(error);
-
-  FOR_EACH_OBSERVER(PrefStore::Observer,
-                    observers_,
-                    OnInitializationCompleted(true));
+  if (on_file_read_interceptor_) {
+    const FinalizePrefsReadCallback finalize_file_read(
+        base::Bind(&JsonPrefStore::FinalizeFileRead, this,
+                   initialization_successful));
+    on_file_read_interceptor_->Run(unprocessed_prefs.Pass(),
+                                   read_only_,
+                                   finalize_file_read);
+    on_file_read_interceptor_.reset();
+  } else {
+    FinalizeFileRead(initialization_successful, unprocessed_prefs.Pass(),
+                     false);
+  }
 }
 
 JsonPrefStore::~JsonPrefStore() {
@@ -337,3 +346,31 @@ bool JsonPrefStore::SerializeData(std::string* output) {
   serializer.set_pretty_print(true);
   return serializer.Serialize(*prefs_);
 }
+
+void JsonPrefStore::FinalizeFileRead(bool initialization_successful,
+                                     scoped_ptr<base::DictionaryValue> prefs,
+                                     bool schedule_write) {
+  if (!initialization_successful) {
+    FOR_EACH_OBSERVER(PrefStore::Observer,
+                      observers_,
+                      OnInitializationCompleted(false));
+    return;
+  }
+
+  prefs_ = prefs.Pass();
+
+  initialized_ = true;
+
+  const bool had_filter_modification =

[Bernhard Bauer, 2014/04/29 15:13:04]

Nit: we don't usually mark local variables as const.

[gab, 2014/04/29 15:46:44]

Sure, I like putting const as much as possible whenever it's not expected to
change -- I prefer being explicit; but I don't really mind in this case.

Done.

+      pref_filter_ && pref_filter_->FilterOnLoad(prefs_.get());
+
+  if ((schedule_write || had_filter_modification) && !read_only_)
+    writer_.ScheduleWrite(this);
+
+  if (error_delegate_.get() && read_error_ != PREF_READ_ERROR_NONE)

[Bernhard Bauer, 2014/04/29 15:13:04]

You could probably leave out the .get().

[gab, 2014/04/29 15:46:44]

Done.

+    error_delegate_->OnError(read_error_);
+
+  FOR_EACH_OBSERVER(PrefStore::Observer,
+                    observers_,
+                    OnInitializationCompleted(true));
+}