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

chrome/browser/ui/prefs_migrator_unittest.cc

+// Copyright (c) 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 "base/observer_list.h"
+#include "base/prefs/writeable_pref_store.h"
+#include "base/values.h"
+#include "chrome/browser/ui/prefs_migrator.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class DictionaryPrefStore : public WriteablePrefStore {
+ public:
+  DictionaryPrefStore() : WriteablePrefStore() {}
+  // Overrides from PrefStore.
+  virtual void AddObserver(Observer* observer) OVERRIDE {
+    observers_.AddObserver(observer);
+  }
+
+  virtual void RemoveObserver(Observer* observer) OVERRIDE {
+    observers_.RemoveObserver(observer);
+  }
+
+  // Get the value for a given preference |key| and stores it in |*result|.

[Bernhard Bauer, 2014/09/10 09:15:47]

Nit: I don't think these comments are necessary.

[gab, 2014/09/10 14:40:04]

+1, no need to copy method comments from derived APIs.

[dgrogan, 2014/09/10 23:10:00]

Done.

[dgrogan, 2014/09/10 23:10:00]

Done.

+  // |*result| is only modified if the return value is true and if |result|
+  // is not NULL. Ownership of the |*result| value remains with the PrefStore.
+  virtual bool GetValue(const std::string& key,
+                        const base::Value** result) const OVERRIDE {
+    const base::Value* tmp = NULL;
+    if (!prefs_.Get(key, &tmp))
+      return false;
+
+    if (result)
+      *result = tmp;
+    return true;

[gab, 2014/09/10 14:40:05]

Doesn't

return prefs_.Get(key, result);

do the same thing as lines 30-36?

[dgrogan, 2014/09/10 23:10:00]

I copied this from JsonPrefStore and was wondering the same thing.

+  }
+
+  // Overrides from WriteablePrefStore.
+  // Sets a |value| for |key| in the store. Assumes ownership of |value|, which
+  // must be non-NULL.
+  virtual void SetValue(const std::string& key, base::Value* value) OVERRIDE {
+    DCHECK(value);
+    scoped_ptr<base::Value> new_value(value);
+    base::Value* old_value = NULL;
+    prefs_.Get(key, &old_value);
+    if (!old_value || !value->Equals(old_value)) {

[gab, 2014/09/10 14:40:04]

Do you really need these checks? Seems overkill to optimize this for tests.

prefs_.Set(key, new_value);
ReportValueChanged(key);

seems sufficient to me.

[dgrogan, 2014/09/10 23:10:00]

Done. Again, I had just copy/pasted from JsonPrefStore.

+      prefs_.Set(key, new_value.release());
+      ReportValueChanged(key);
+    }
+  }
+
+  // Removes the value for |key|.
+  virtual void RemoveValue(const std::string& key) OVERRIDE {
+    if (prefs_.RemovePath(key, NULL))
+      ReportValueChanged(key);
+  }
+
+  // Equivalent to PrefStore::GetValue but returns a mutable value.
+  virtual bool GetMutableValue(const std::string& key,
+                               base::Value** result) OVERRIDE {
+    return prefs_.Get(key, result);
+  }
+
+  // Triggers a value changed notification. This function needs to be called
+  // if one retrieves a list or dictionary with GetMutableValue and change its
+  // value. SetValue takes care of notifications itself. Note that
+  // ReportValueChanged will trigger notifications even if nothing has changed.
+  virtual void ReportValueChanged(const std::string& key) OVERRIDE {
+    NOTIMPLEMENTED();
+  }
+
+  // Same as SetValue, but doesn't generate notifications. This is used by
+  // PrefService::GetMutableUserPref() in order to put empty entries
+  // into the user pref store. Using SetValue is not an option since existing
+  // tests rely on the number of notifications generated.
+  virtual void SetValueSilently(const std::string& key,
+                                base::Value* value) OVERRIDE {
+    NOTIMPLEMENTED();
+  }
+
+  void SignalObservers() {
+    FOR_EACH_OBSERVER(
+        PrefStore::Observer, observers_, OnInitializationCompleted(true));
+  }
+
+ private:
+  virtual ~DictionaryPrefStore() {}
+
+  base::DictionaryValue prefs_;
+  ObserverList<PrefStore::Observer, true> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(DictionaryPrefStore);
+};
+
+}  // unnamed namespace

[Bernhard Bauer, 2014/09/10 09:15:47]

Nit: Usually the closing comment for a namespace is exactly the declaration that
opened it, i.e. just "namespace" here.

[dgrogan, 2014/09/10 23:10:00]

Done.

+
+TEST(UIPrefsMigratorTest, First) {
+  scoped_refptr<DictionaryPrefStore> pref_store(new DictionaryPrefStore);
+}