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

chrome/browser/prefs/chrome_pref_service_factory.cc

 // 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.
 
 #include "chrome/browser/prefs/chrome_pref_service_factory.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/debug/trace_event.h"
 #include "base/files/file_path.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/default_pref_store.h"
 #include "base/prefs/json_pref_store.h"
 #include "base/prefs/pref_filter.h"
 #include "base/prefs/pref_notifier_impl.h"
 #include "base/prefs/pref_registry.h"
 #include "base/prefs/pref_registry_simple.h"
 #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"
 #include "chrome/browser/prefs/command_line_pref_store.h"
 #include "chrome/browser/prefs/pref_hash_filter.h"
 #include "chrome/browser/prefs/pref_model_associator.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prefs/pref_service_syncable_factory.h"
 #include "chrome/browser/prefs/profile_pref_store_manager.h"
 #include "chrome/browser/profiles/file_path_verifier_win.h"
@@ skipping 363 matching lines @@
 
   factory->set_async(async);
   factory->set_extension_prefs(extension_prefs);
   factory->set_command_line_prefs(
       make_scoped_refptr(
           new CommandLinePrefStore(CommandLine::ForCurrentProcess())));
   factory->set_read_error_callback(base::Bind(&HandleReadError));
   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 {
 
 namespace internals {
 
 // Group modifications should be reflected in first_run_browsertest.cc and
 // pref_hash_browsertest.cc.
 const char kSettingsEnforcementTrialName[] = "SettingsEnforcement";
 const char kSettingsEnforcementGroupNoEnforcement[] = "no_enforcement";
@@ skipping 38 matching lines @@
   // done since sync may bring back the user's server value post-reset which
   // could potentially cause a "settings flash" between the factory default and
   // the re-instantiated server value. Starting sync ASAP minimizes the window
   // before the server value is re-instantiated (this window can otherwise be
   // as long as 10 seconds by default).
   const base::Closure start_sync_flare_for_prefs =
       base::Bind(sync_start_util::GetFlareForSyncableService(profile_path),
                  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 =
       factory.CreateSyncable(pref_registry.get());
 
   ConfigureDefaultSearchPrefMigrationToDictionaryValue(pref_service.get());
 
   return pref_service.Pass();
 }
 
@@ skipping 33 matching lines @@
 
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   ProfilePrefStoreManager::RegisterProfilePrefs(registry);
 }
 
 void RegisterPrefs(PrefRegistrySimple* registry) {
   ProfilePrefStoreManager::RegisterPrefs(registry);
 }
 
 }  // namespace chrome_prefs