No longer register app window placement preference keys on the fly.

chrome/browser/prefs/chrome_pref_service_factory.cc

Index: chrome/browser/prefs/chrome_pref_service_factory.cc
diff --git a/chrome/browser/prefs/chrome_pref_service_factory.cc b/chrome/browser/prefs/chrome_pref_service_factory.cc
index 81b2c6670c8f92deaf05cf5067846753f6acb13f..11d8bab0994c424797850f5dbfe33850696a0cc0 100644
--- a/chrome/browser/prefs/chrome_pref_service_factory.cc
+++ b/chrome/browser/prefs/chrome_pref_service_factory.cc
@@ -22,6 +22,7 @@
 #include "base/prefs/pref_service.h"
 #include "base/prefs/pref_store.h"
 #include "base/prefs/pref_value_store.h"
+#include "base/strings/string_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
@@ -405,6 +406,69 @@ void PrepareFactory(
   factory->set_user_prefs(user_pref_store);
 }
 
+class BrowserUIPrefsMigrationObserver : public PrefStore::Observer {

[gab, 2014/09/08 21:29:25]

I think this should live in its own file:
 1) to avoid polluting chrome_pref_service_factory.cc
 2) to allow unit testing (I don't think you'd actually need a full blown
browser test in this case, unittests should suffice, mocked by an in memory
DictionaryPrefStore rather than having to use IO).

[gab, 2014/09/08 21:29:25]

Add a meta comment explaining the purpose of this class:
 1) Why it can't do typical migration like everybody else (migration problem).
 2) A note that it deletes itself when done.
 3) A todo to remove this code in say M42.

[dgrogan, 2014/09/09 03:15:43]

Moved to its own file.

[dgrogan, 2014/09/09 03:15:43]

Done.

+ public:
+  explicit BrowserUIPrefsMigrationObserver(
+      scoped_refptr<PersistentPrefStore> pref_store)
+      : pref_store_(pref_store) {}
+  virtual void OnPrefValueChanged(const std::string& key) OVERRIDE {}
+  virtual void OnInitializationCompleted(bool succeeded) OVERRIDE;
+ private:

[gab, 2014/09/08 21:29:25]

space above.

[dgrogan, 2014/09/09 03:15:43]

Done.

+  scoped_refptr<PersistentPrefStore> pref_store_;
+};

[gab, 2014/09/08 21:29:25]

DISALLOW_COPY_AND_ASSIGN above.

[dgrogan, 2014/09/09 03:15:43]

Done.

+
+void BrowserUIPrefsMigrationObserver::OnInitializationCompleted(
+    bool succeeded) {
+  pref_store_->RemoveObserver(this);
+  scoped_ptr<BrowserUIPrefsMigrationObserver> self_deleter(this);
+  if (!succeeded)
+    return;
+
+  base::Value* browser_value = NULL;
+  if (!pref_store_->GetMutableValue("browser", &browser_value)) {
+    return;
+  }
+  base::DictionaryValue* browser_dict = NULL;
+  if (!browser_value->GetAsDictionary(&browser_dict))
+    return;
+
+  // Don't bother scanning "browser" if the migration already occurred.
+  if (browser_dict->HasKey(prefs::kAppWindowPlacement))
+    return;
+
+  // Get a set of keys in the dictionary. This must be done separately from the
+  // migration because the migration modifies the dictionary being iterated.
+  std::set<std::string> keys_to_check;
+  for (base::DictionaryValue::Iterator it(*browser_dict); !it.IsAtEnd();
+       it.Advance()) {
+    keys_to_check.insert(it.key());
+  }
+
+  scoped_ptr<base::DictionaryValue> app_window_placement;

[gab, 2014/09/08 21:29:25]

Add a comment about how those pref used to look like for future readers to
understand this migration code without knowing about the old state of the world.

[dgrogan, 2014/09/09 03:15:43]

Done.

+  const std::string search_for =
+      std::string(prefs::kBrowserWindowPlacement) + "_";
+  for (std::set<std::string>::const_iterator it = keys_to_check.begin();
+       it != keys_to_check.end();
+       ++it) {
+    std::string full_key("browser." + *it);
+    if (StartsWithASCII(full_key, search_for, true /* case_sensitive */)) {
+      if (full_key == prefs::kBrowserWindowPlacementPopup)
+        continue;
+      scoped_ptr<base::Value> single_app_placement_dict;
+      bool found = browser_dict->Remove(*it, &single_app_placement_dict);
+      DCHECK(found);
+      std::string new_key(full_key.substr(search_for.length()));
+      if (!app_window_placement)
+        app_window_placement.reset(new base::DictionaryValue);
+      app_window_placement->Set(new_key, single_app_placement_dict.release());
+    }
+  }
+  if (app_window_placement) {
+    pref_store_->SetValue(prefs::kAppWindowPlacement,
+                          app_window_placement.release());
+  }
+}
+
 }  // namespace
 
 namespace chrome_prefs {
@@ -463,14 +527,17 @@ scoped_ptr<PrefServiceSyncable> CreateProfilePrefs(
                  syncer::PREFERENCES);
 
   PrefServiceSyncableFactory factory;
+  scoped_refptr<PersistentPrefStore> user_pref_store(
+      CreateProfilePrefStoreManager(profile_path)
+          ->CreateProfilePrefStore(pref_io_task_runner,
+                                   start_sync_flare_for_prefs,
+                                   validation_delegate));
+  user_pref_store->AddObserver(
+      new BrowserUIPrefsMigrationObserver(user_pref_store));

[gab, 2014/09/08 21:29:25]

Add a comment above that the BrowserUIPrefsMigrationObserver deletes itself when
done.

[dgrogan, 2014/09/09 03:15:43]

Done.

   PrepareFactory(&factory,
                  policy_service,
                  supervised_user_settings,
-                 scoped_refptr<PersistentPrefStore>(
-                     CreateProfilePrefStoreManager(profile_path)
-                         ->CreateProfilePrefStore(pref_io_task_runner,
-                                                  start_sync_flare_for_prefs,
-                                                  validation_delegate)),
+                 user_pref_store,
                  extension_prefs,
                  async);
   scoped_ptr<PrefServiceSyncable> pref_service =