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

chrome/browser/prefs/tracked/tracked_preferences_migration.cc

+// Copyright 2014 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 "chrome/browser/prefs/tracked/tracked_preferences_migration.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/prefs/json_pref_store.h"
+#include "base/values.h"
+
+namespace {
+
+class TrackedPreferencesMigrator
+    : public base::RefCounted<TrackedPreferencesMigrator> {
+ public:
+  enum PrefStoreID {

[Bernhard Bauer, 2014/04/29 16:38:53]

This could be private, right?

[gab, 2014/04/29 20:12:14]

Done.

+    UNPROTECTED_PREFERENCE_STORE,
+    PROTECTED_PREFERENCE_STORE
+  };
+
+  // Constructs a TrackedPreferencesMigrator which sets itself up as the
+  // OnFileReadInterceptor for both |unprotected_pref_store| and
+  // |protected_pref_store|. It guarantees that the most recent pref value
+  // for |unprotected_pref_names| and |protected_pref_names| are in the
+  // appropriate JsonPrefStores before those stores even sees those values.
+  TrackedPreferencesMigrator(
+      const std::set<std::string>& unprotected_pref_names,
+      const std::set<std::string>& protected_pref_names,
+      JsonPrefStore* unprotected_pref_store,
+      JsonPrefStore* protected_pref_store);
+
+  ~TrackedPreferencesMigrator();

[Bernhard Bauer, 2014/04/29 16:38:53]

Destructors should be private for refcounted classes. No one else should be able
to delete a refcounted object.

[gab, 2014/04/29 20:12:14]

That's what I wanted to do, but it doesn't work because it's the callbacks that
end up owning this class (and thus deleting it); technically those callbacks
will run in this class so you're right that the delete will happen in this
class. However the compiler can't figure  this out when a reference is handed in
base::Bind below and won't compile unless this destructor is public :(.

[Bernhard Bauer, 2014/04/29 21:20:28]

You'll need to friend the base class, i.e.
base::RefCounted<TrackedPreferencesMigrator>. I admit that it's a bit awkward to
write, but that is definitely the common style. I also think there's a presubmit
check / Clang plugin that will complain about public destructors for refcounted
classes.

[gab, 2014/04/29 22:36:05]

Ah I see, done, thanks!

+
+ private:
+  // Stores the data coming in for the store identified by |id| into this class
+  // and then calls MigrateIfReady();
+  void OnFileRead(
+      PrefStoreID id,
+      scoped_ptr<base::DictionaryValue> prefs,
+      bool read_only,
+      const JsonPrefStore::FinalizePrefsReadCallback& finalize_prefs_read);
+
+  // Proceeds with migration if both |unprotected_prefs_| and |protected_prefs_|
+  // have been set.
+  void MigrateIfReady();
+
+  // Copies the value of each pref in |pref_names| which is set in |old_store|,
+  // but not in |new_store| into |new_store|. Sets |old_store_needs_cleanup| to
+  // true if any old duplicates remain in |old_store| and sets
+  // |new_store_altered| to true if any value was copied to |new_store|.
+  static void MigratePrefsFromOldToNewStore(

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

I think this could live outside of the class if it's private and static anyway.

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

I debated both and went this way because it was slightly more encapsulated
functionality and it kept this method close to its only caller (making it
slightly more readable IMO).

Otherwise I'd have to put in the anonymous namespace above and a reader would
have to bounce back-and-forth -- our style guide also says that type
declarations (i.e. classes) should come before methods IIRC which means that I
would technically have to put it at the bottom of this namespace, but then I'd
have to forward-declare it for it to be usable by this class...

[Bernhard Bauer, 2014/04/29 16:38:53]

Hm, but class declarations don't need to be grouped with their implementations
-- usually the former is in the header and the latter in the .cc file anyway.
You have a similar issue if you have two classes (in an anonymous namespace)
that reference each other: the declarations for both classes need to come first,
and then the implementations. In fact, being able to reference a class before
it's implemented is the reason why I usually don't just fully inline classes in
anonymous namespaces.

But anyway, at this point it's probably just a matter of personal preference. It
should be possible to move the method out of the class and still adhere to the
style guide, so that should not keep you, but if you prefer to do it some other
way, I won't object either.

[gab, 2014/04/29 20:12:14]

Ok, done. I put the free-method between TrackedPreferencesMigrator's declaration
and definition. That feels a bit weird to me, but I don't mind if you feel
that's better.

+      const std::set<std::string>& pref_names,
+      const base::DictionaryValue* old_store,
+      base::DictionaryValue* new_store,
+      bool* old_store_needs_cleanup,
+      bool* new_store_altered);
+
+  const std::set<std::string> unprotected_pref_names_;
+  const std::set<std::string> protected_pref_names_;
+
+  scoped_ptr<base::DictionaryValue> unprotected_prefs_;
+  scoped_ptr<base::DictionaryValue> protected_prefs_;
+
+  JsonPrefStore::FinalizePrefsReadCallback finalize_unprotected_prefs_read_;
+  JsonPrefStore::FinalizePrefsReadCallback finalize_protected_prefs_read_;
+
+  // Set to true if either store is reported to be read-only (in which case the
+  // migration will be copy only -- no deletions).
+  bool read_only_;
+
+  DISALLOW_COPY_AND_ASSIGN(TrackedPreferencesMigrator);
+};
+
+TrackedPreferencesMigrator::TrackedPreferencesMigrator(
+    const std::set<std::string>& unprotected_pref_names,
+    const std::set<std::string>& protected_pref_names,
+      JsonPrefStore* unprotected_pref_store,

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

Nit: Unindent by two spaces.

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

Done.

+      JsonPrefStore* protected_pref_store)
+        : unprotected_pref_names_(unprotected_pref_names),

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

Unindent this line and the following ones too, by two more spaces.

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

I thought we wanted an extra 4 spaces here, but git cl format seems to disagree
with me and put it flush with the params.

+          protected_pref_names_(protected_pref_names),
+          read_only_(false) {
+  // The callbacks binded below will own this TrackedPreferencesMigrator by

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

Nit: "bound"

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

Done.

+  // reference.
+  unprotected_pref_store->InterceptNextFileRead(
+      base::Bind(&TrackedPreferencesMigrator::OnFileRead, this,
+                 UNPROTECTED_PREFERENCE_STORE));
+  protected_pref_store->InterceptNextFileRead(
+      base::Bind(&TrackedPreferencesMigrator::OnFileRead, this,
+                 PROTECTED_PREFERENCE_STORE));
+}
+
+TrackedPreferencesMigrator::~TrackedPreferencesMigrator() {}
+
+void TrackedPreferencesMigrator::OnFileRead(
+    PrefStoreID id,
+    scoped_ptr<base::DictionaryValue> prefs,
+    bool read_only,
+    const JsonPrefStore::FinalizePrefsReadCallback& finalize_prefs_read) {
+  if (read_only)
+    read_only_ = true;
+
+  switch (id) {
+    case UNPROTECTED_PREFERENCE_STORE:
+      unprotected_prefs_ = prefs.Pass();
+      finalize_unprotected_prefs_read_ = finalize_prefs_read;
+      break;
+    case PROTECTED_PREFERENCE_STORE:
+      protected_prefs_ = prefs.Pass();
+      finalize_protected_prefs_read_ = finalize_prefs_read;
+      break;
+  }
+
+  MigrateIfReady();
+}
+
+void TrackedPreferencesMigrator::MigrateIfReady() {
+  // Wait for both stores to have been read before proceeding.
+  if (!protected_prefs_ || !unprotected_prefs_)
+    return;
+
+  bool protected_prefs_need_cleanup = false;
+  bool unprotected_prefs_altered = false;
+  MigratePrefsFromOldToNewStore(unprotected_pref_names_,
+                                protected_prefs_.get(),
+                                unprotected_prefs_.get(),
+                                &protected_prefs_need_cleanup,
+                                &unprotected_prefs_altered);
+  bool unprotected_prefs_need_cleanup = false;
+  bool protected_prefs_altered = false;
+  MigratePrefsFromOldToNewStore(protected_pref_names_,
+                                unprotected_prefs_.get(),
+                                protected_prefs_.get(),
+                                &unprotected_prefs_need_cleanup,
+                                &protected_prefs_altered);
+
+  if (!read_only_) {
+    // TODO(gab): Register callbacks to cleanup values from their old store when
+    // we get confirmation that the copies were successfully flushed out to
+    // their new respective stores.
+  }
+
+  // Finally, hand the prefs back to their respective JsonPrefStore.
+  finalize_unprotected_prefs_read_.Run(unprotected_prefs_.Pass(),
+                                       unprotected_prefs_altered);
+  finalize_protected_prefs_read_.Run(protected_prefs_.Pass(),
+                                     protected_prefs_altered);
+}
+
+// static
+void TrackedPreferencesMigrator::MigratePrefsFromOldToNewStore(
+    const std::set<std::string>& pref_names,
+    const base::DictionaryValue* old_store,
+    base::DictionaryValue* new_store,
+    bool* old_store_needs_cleanup,
+    bool* new_store_altered) {
+  const base::Value* tmp = NULL;

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

Could you come up with a better name for this variable than |tmp|? |old_value|
maybe?

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

Done.

+  for (std::set<std::string>::const_iterator it = pref_names.begin();
+       it != pref_names.end(); ++it) {
+    const std::string& pref_name = *it;
+    if (old_store->Get(pref_name, &tmp)) {

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

You could probably invert the conditions here and `continue` instead of nesting
if-statements.

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

This nesting is required to set |old_store_needs_cleanup|.

It is technically possible that the values would end up in both stores following
a crash (as highlighted by the comment below) -- in which case we would still
need to cleanup |old_store| despite |new_store| already having a value.

[Bernhard Bauer, 2014/04/29 16:38:53]

Hm, wouldn't the following work?

  if (!old_store->Get(pref_name, &tmp))
    continue;

  *old_store_needs_cleanup = true;
  if (new_store->Get(pref_name, &tmp))
    continue;
  
  [...]

It saves you two levels of indenting.

[gab, 2014/04/29 20:12:14]

Not sure which one I prefer; I've heard of people trying to minimize return
statements in a simple method to make for nicer assembly (reducing stack popping
complexity) and I guess the same applies to continue.

Otherwise I do agree that it's slightly easier to read with the continues.

Done.

[Bernhard Bauer, 2014/04/29 21:20:28]

Huh. I would have assumed that the compiler would produce the same machine code,
as both versions are functionally equivalent. 

+      *old_store_needs_cleanup = true;
+      if (!new_store->Get(pref_name, &tmp)) {
+        // Copy the value from |old_store| to |new_store| rather than moving it
+        // to avoid data loss should |old_store| be flushed to disk without
+        // |new_store| having equivalently been successfully flushed to disk
+        // on crash.
+        new_store->Set(pref_name, tmp->DeepCopy());
+        *new_store_altered = true;
+      }
+    }
+  }
+}
+
+}  // namespace
+
+void SetupTrackedPrefererencesMigration(
+    const std::set<std::string>& unprotected_pref_names,
+    const std::set<std::string>& protected_pref_names,
+    JsonPrefStore* unprotected_pref_store,
+    JsonPrefStore* protected_pref_store) {
+  scoped_refptr<TrackedPreferencesMigrator> prefs_migrator(
+      new TrackedPreferencesMigrator(unprotected_pref_names,
+                                     protected_pref_names,
+                                     unprotected_pref_store,
+                                     protected_pref_store));
+}