A subtle bug was exposed when the following was added to Preferences::RegisterUserPrefs:

  prefs->RegisterIntegerPref(prefs::kLabsTalkEnabled, 0);

Because prefs::kLabsTalkEnabled = "extensions.settings.ggnioahjipcehijkhpdjekioddnjoben.state",
"extensions.settings" in the DEFAULT pref store is forced to a DictionaryValue
instead of an empty value so that data can be stored in it. This invalidaded a
test in PrefService::GetMutableDictionary, causing GetMutableDictionary to
return a pointer to a DictionaryValue in the default pref store instead of
creating a dictionary in the user pref store.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=7066
TEST=See bug; should no longer repro. Also, test preferences in general.

Review URL: http://codereview.chromium.org/3533008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61209 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 43 insertions, 19 deletions

diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc
index 0668ed4..d807f5a4 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -502,7 +502,9 @@ DictionaryValue* PrefService::GetMutableDictionary(const char* path) {
 
   DictionaryValue* dict = NULL;
   Value* tmp_value = NULL;
-  if (!pref_value_store_->GetValue(path, &tmp_value) ||
+  // Look for an existing preference in the user store. If it doesn't
+  // exist or isn't the correct type, create a new user preference.
+  if (!pref_value_store_->GetUserValue(path, &tmp_value) ||
       !tmp_value->IsType(Value::TYPE_DICTIONARY)) {
     dict = new DictionaryValue;
     pref_value_store_->SetUserPrefValue(path, dict);
@@ -527,7 +529,9 @@ ListValue* PrefService::GetMutableList(const char* path) {
 
   ListValue* list = NULL;
   Value* tmp_value = NULL;
-  if (!pref_value_store_->GetValue(path, &tmp_value) ||
+  // Look for an existing preference in the user store. If it doesn't
+  // exist or isn't the correct type, create a new user preference.
+  if (!pref_value_store_->GetUserValue(path, &tmp_value) ||
       !tmp_value->IsType(Value::TYPE_LIST)) {
     list = new ListValue;
     pref_value_store_->SetUserPrefValue(path, list);
diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h
index 3e90db6..f1128a22 100644
--- a/chrome/browser/prefs/pref_service.h
+++ b/chrome/browser/prefs/pref_service.h
@@ -194,6 +194,8 @@ class PrefService : public NonThreadSafe {
   // WARNING: Changes to the dictionary or list will not automatically notify
   // pref observers.
   // Use a ScopedPrefUpdate to update observers on changes.
+  // These should really be GetUserMutable... since we will only ever get
+  // a mutable from the user preferences store.
   DictionaryValue* GetMutableDictionary(const char* path);
   ListValue* GetMutableList(const char* path);
 
diff --git a/chrome/browser/prefs/pref_value_store.cc b/chrome/browser/prefs/pref_value_store.cc
index 46c825d..ff2a7a4 100644
--- a/chrome/browser/prefs/pref_value_store.cc
+++ b/chrome/browser/prefs/pref_value_store.cc
@@ -66,21 +66,20 @@ bool PrefValueStore::GetValue(const std::string& name,
                               Value** out_value) const {
   // Check the |PrefStore|s in order of their priority from highest to lowest
   // to find the value of the preference described by the given preference name.
-  // If the found value is not the correct type, keep looking. This allows a
-  // default value to override an outdated user-pref setting.
   for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) {
-    if (pref_stores_[i].get() &&
-        pref_stores_[i]->prefs()->Get(name.c_str(), out_value) &&
-        IsValidType(GetRegisteredType(name), (*out_value)->GetType(),
-                    static_cast<PrefNotifier::PrefStoreType>(i))) {
+    if (GetValueFromStore(name.c_str(),
+                          static_cast<PrefNotifier::PrefStoreType>(i),
+                          out_value))
       return true;
-    }
   }
-  // No valid value found for the given preference name: set the return false.
-  *out_value = NULL;
   return false;
 }
 
+bool PrefValueStore::GetUserValue(const std::string& name,
+                                  Value** out_value) const {
+  return GetValueFromStore(name.c_str(), PrefNotifier::USER_STORE, out_value);
+}
+
 void PrefValueStore::RegisterPreferenceType(const std::string& name,
                                             Value::ValueType type) {
   pref_types_[name] = type;
@@ -225,17 +224,28 @@ PrefNotifier::PrefStoreType PrefValueStore::ControllingPrefStoreForPref(
   return PrefNotifier::INVALID_STORE;
 }
 
-bool PrefValueStore::PrefValueInStore(const char* name,
-                                      PrefNotifier::PrefStoreType store) const {
-  if (!pref_stores_[store].get()) {
-    // No store of that type, so this pref can't be in it.
-    return false;
-  }
+bool PrefValueStore::PrefValueInStore(
+    const char* name,
+    PrefNotifier::PrefStoreType store) const {
+  // Declare a temp Value* and call GetValueFromStore,
+  // ignoring the output value.
   Value* tmp_value = NULL;
-  if (pref_stores_[store]->prefs()->Get(name, &tmp_value) &&
-      IsValidType(GetRegisteredType(name), tmp_value->GetType(), store)) {
+  return GetValueFromStore(name, store, &tmp_value);
+}
+
+bool PrefValueStore::GetValueFromStore(
+    const char* name,
+    PrefNotifier::PrefStoreType store,
+    Value** out_value) const {
+  // Only return true if we find a value and it is the correct type, so stale
+  // values with the incorrect type will be ignored.
+  if (pref_stores_[store].get() &&
+      pref_stores_[store]->prefs()->Get(name, out_value) &&
+      IsValidType(GetRegisteredType(name), (*out_value)->GetType(), store)) {
     return true;
   }
+  // No valid value found for the given preference name: set the return false.
+  *out_value = NULL;
   return false;
 }
 
diff --git a/chrome/browser/prefs/pref_value_store.h b/chrome/browser/prefs/pref_value_store.h
index c361e07..1dd4abb 100644
--- a/chrome/browser/prefs/pref_value_store.h
+++ b/chrome/browser/prefs/pref_value_store.h
@@ -56,6 +56,9 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // Preference::GetValue() instead of calling this method directly.
   bool GetValue(const std::string& name, Value** out_value) const;
 
+  // Same as GetValue but only searches USER_STORE.
+  bool GetUserValue(const std::string& name, Value** out_value) const;
+
   // Adds a preference to the mapping of names to types.
   void RegisterPreferenceType(const std::string& name, Value::ValueType type);
 
@@ -198,6 +201,11 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   bool PrefValueInStore(const char* name,
                         PrefNotifier::PrefStoreType store) const;
 
+  // Get a value from the specified store type.
+  bool GetValueFromStore(const char* name,
+                         PrefNotifier::PrefStoreType store,
+                         Value** out_value) const;
+
   // Called during policy refresh after ReadPrefs completes on the thread
   // that initiated the policy refresh. RefreshPolicyPrefsCompletion takes
   // ownership of the |callback| object.