diff options
| -rw-r--r-- | chrome/browser/prefs/pref_service.cc | 56 | ||||
| -rw-r--r-- | chrome/browser/prefs/pref_service.h | 9 | ||||
| -rw-r--r-- | chrome/browser/prefs/pref_value_store.cc | 57 | ||||
| -rw-r--r-- | chrome/browser/prefs/pref_value_store.h | 31 | ||||
| -rw-r--r-- | chrome/browser/prefs/pref_value_store_unittest.cc | 79 | ||||
| -rw-r--r-- | chrome/browser/sync/glue/preference_model_associator_unittest.cc | 2 |
6 files changed, 172 insertions, 62 deletions
diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc index bc87c2f..0668ed4 100644 --- a/chrome/browser/prefs/pref_service.cc +++ b/chrome/browser/prefs/pref_service.cc @@ -312,10 +312,7 @@ bool PrefService::HasPrefPath(const char* path) const { const PrefService::Preference* PrefService::FindPreference( const char* pref_name) const { DCHECK(CalledOnValidThread()); - // We only look up prefs by name, so the type is irrelevant, except that it - // must be one that Preference() allows (i.e., neither TYPE_NULL nor - // TYPE_BINARY). - Preference p(this, pref_name, Value::TYPE_INTEGER); + Preference p(this, pref_name); PreferenceSet::const_iterator it = prefs_.find(&p); return it == prefs_.end() ? NULL : *it; } @@ -377,10 +374,13 @@ void PrefService::RegisterPreference(const char* path, Value* default_value) { return; } + Value::ValueType orig_type = default_value->GetType(); + DCHECK(orig_type != Value::TYPE_NULL && orig_type != Value::TYPE_BINARY) << + "invalid preference type: " << orig_type; + // We set the default value of dictionaries and lists to be null so it's // easier for callers to check for empty dict/list prefs. The PrefValueStore // accepts ownership of the value (null or default_value). - Value::ValueType orig_type = default_value->GetType(); if (Value::TYPE_LIST == orig_type || Value::TYPE_DICTIONARY == orig_type) { pref_value_store_->SetDefaultPrefValue(path, Value::CreateNullValue()); } else { @@ -388,7 +388,8 @@ void PrefService::RegisterPreference(const char* path, Value* default_value) { pref_value_store_->SetDefaultPrefValue(path, scoped_value.release()); } - prefs_.insert(new Preference(this, path, orig_type)); + pref_value_store_->RegisterPreferenceType(path, orig_type); + prefs_.insert(new Preference(this, path)); } void PrefService::ClearPref(const char* path) { @@ -416,11 +417,12 @@ void PrefService::Set(const char* path, const Value& value) { // user values. bool value_changed = false; if (value.GetType() == Value::TYPE_NULL && - (pref->type() == Value::TYPE_DICTIONARY || - pref->type() == Value::TYPE_LIST)) { + (pref->GetType() == Value::TYPE_DICTIONARY || + pref->GetType() == Value::TYPE_LIST)) { value_changed = pref_value_store_->RemoveUserPrefValue(path); - } else if (pref->type() != value.GetType()) { - NOTREACHED() << "Trying to set pref " << path << " of type " << pref->type() + } else if (pref->GetType() != value.GetType()) { + NOTREACHED() << "Trying to set pref " << path + << " of type " << pref->GetType() << " to value of type " << value.GetType(); } else { value_changed = pref_value_store_->SetUserPrefValue(path, value.DeepCopy()); @@ -493,7 +495,7 @@ DictionaryValue* PrefService::GetMutableDictionary(const char* path) { NOTREACHED() << "Trying to get an unregistered pref: " << path; return NULL; } - if (pref->type() != Value::TYPE_DICTIONARY) { + if (pref->GetType() != Value::TYPE_DICTIONARY) { NOTREACHED() << "Wrong type for GetMutableDictionary: " << path; return NULL; } @@ -518,7 +520,7 @@ ListValue* PrefService::GetMutableList(const char* path) { NOTREACHED() << "Trying to get an unregistered pref: " << path; return NULL; } - if (pref->type() != Value::TYPE_LIST) { + if (pref->GetType() != Value::TYPE_LIST) { NOTREACHED() << "Wrong type for GetMutableList: " << path; return NULL; } @@ -555,8 +557,9 @@ void PrefService::SetUserPrefValue(const char* path, Value* new_value) { NOTREACHED() << "Preference is managed: " << path; return; } - if (pref->type() != new_value->GetType()) { - NOTREACHED() << "Trying to set pref " << path << " of type " << pref->type() + if (pref->GetType() != new_value->GetType()) { + NOTREACHED() << "Trying to set pref " << path + << " of type " << pref->GetType() << " to value of type " << new_value->GetType(); return; } @@ -569,15 +572,15 @@ void PrefService::SetUserPrefValue(const char* path, Value* new_value) { // PrefService::Preference PrefService::Preference::Preference(const PrefService* service, - const char* name, - Value::ValueType type) - : type_(type), - name_(name), + const char* name) + : name_(name), pref_service_(service) { DCHECK(name); DCHECK(service); - DCHECK(type != Value::TYPE_NULL && type != Value::TYPE_BINARY) << - "invalid preference type: " << type; +} + +Value::ValueType PrefService::Preference::GetType() const { + return pref_service_->pref_value_store_->GetRegisteredType(name_); } const Value* PrefService::Preference::GetValue() const { @@ -585,18 +588,11 @@ const Value* PrefService::Preference::GetValue() const { "Must register pref before getting its value"; Value* found_value = NULL; - if (pref_service_->pref_value_store_->GetValue(name_, &found_value)) { - Value::ValueType found_type = found_value->GetType(); - // Dictionaries and lists have default values of TYPE_NULL. - if (found_type == type_ || - (found_type == Value::TYPE_NULL - && (type_ == Value::TYPE_LIST || type_ == Value::TYPE_DICTIONARY))) { - return found_value; - } - } + if (pref_service_->pref_value_store_->GetValue(name_, &found_value)) + return found_value; // Every registered preference has at least a default value. - NOTREACHED() << "no default value for registered pref " << name_; + NOTREACHED() << "no valid value found for registered pref " << name_; return NULL; } diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h index 3fd11be..450c6c1 100644 --- a/chrome/browser/prefs/pref_service.h +++ b/chrome/browser/prefs/pref_service.h @@ -33,16 +33,16 @@ class PrefService : public NonThreadSafe { // dictionary (a branch), or list. You shouldn't need to construct this on // your own; use the PrefService::Register*Pref methods instead. Preference(const PrefService* service, - const char* name, - Value::ValueType type); + const char* name); ~Preference() {} - Value::ValueType type() const { return type_; } - // Returns the name of the Preference (i.e., the key, e.g., // browser.window_placement). const std::string name() const { return name_; } + // Returns the registered type of the preference. + Value::ValueType GetType() const; + // Returns the value of the Preference, falling back to the registered // default value if no other has been set. const Value* GetValue() const; @@ -81,7 +81,6 @@ class PrefService : public NonThreadSafe { private: friend class PrefService; - Value::ValueType type_; std::string name_; // Reference to the PrefService in which this pref was created. diff --git a/chrome/browser/prefs/pref_value_store.cc b/chrome/browser/prefs/pref_value_store.cc index 1dd5a90..46c825d 100644 --- a/chrome/browser/prefs/pref_value_store.cc +++ b/chrome/browser/prefs/pref_value_store.cc @@ -4,7 +4,6 @@ #include "chrome/browser/prefs/pref_value_store.h" -#include "base/values.h" #include "chrome/browser/chrome_thread.h" #include "chrome/browser/extensions/extension_pref_store.h" #include "chrome/browser/policy/configuration_policy_pref_store.h" @@ -13,6 +12,27 @@ #include "chrome/common/json_pref_store.h" #include "chrome/common/notification_service.h" +namespace { + +// Returns true if the actual value is a valid type for the expected type when +// found in the given store. +bool IsValidType(Value::ValueType expected, Value::ValueType actual, + PrefNotifier::PrefStoreType store) { + if (expected == actual) + return true; + + // Dictionaries and lists are allowed to hold TYPE_NULL values too, but only + // in the default pref store. + if (store == PrefNotifier::DEFAULT_STORE && + actual == Value::TYPE_NULL && + (expected == Value::TYPE_DICTIONARY || expected == Value::TYPE_LIST)) { + return true; + } + return false; +} + +} // namespace + // static PrefValueStore* PrefValueStore::CreatePrefValueStore( const FilePath& pref_filename, @@ -46,17 +66,34 @@ 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)) { + pref_stores_[i]->prefs()->Get(name.c_str(), out_value) && + IsValidType(GetRegisteredType(name), (*out_value)->GetType(), + static_cast<PrefNotifier::PrefStoreType>(i))) { return true; } } - // No value found for the given preference name: set the return false. + // No valid value found for the given preference name: set the return false. *out_value = NULL; return false; } +void PrefValueStore::RegisterPreferenceType(const std::string& name, + Value::ValueType type) { + pref_types_[name] = type; +} + +Value::ValueType PrefValueStore::GetRegisteredType( + const std::string& name) const { + PrefTypeMap::const_iterator found = pref_types_.find(name); + if (found == pref_types_.end()) + return Value::TYPE_NULL; + return found->second; +} + bool PrefValueStore::WritePrefs() { bool success = true; for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) { @@ -189,13 +226,17 @@ PrefNotifier::PrefStoreType PrefValueStore::ControllingPrefStoreForPref( } bool PrefValueStore::PrefValueInStore(const char* name, - PrefNotifier::PrefStoreType type) const { - if (!pref_stores_[type].get()) { - // No store of that type set, so this pref can't be in it. + PrefNotifier::PrefStoreType store) const { + if (!pref_stores_[store].get()) { + // No store of that type, so this pref can't be in it. return false; } - Value* tmp_value; - return pref_stores_[type]->prefs()->Get(name, &tmp_value); + Value* tmp_value = NULL; + if (pref_stores_[store]->prefs()->Get(name, &tmp_value) && + IsValidType(GetRegisteredType(name), tmp_value->GetType(), store)) { + return true; + } + return false; } void PrefValueStore::RefreshPolicyPrefsCompletion( diff --git a/chrome/browser/prefs/pref_value_store.h b/chrome/browser/prefs/pref_value_store.h index 8ea4236..ba2308d 100644 --- a/chrome/browser/prefs/pref_value_store.h +++ b/chrome/browser/prefs/pref_value_store.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_PREFS_PREF_VALUE_STORE_H_ #pragma once +#include <map> #include <string> #include <vector> @@ -14,6 +15,7 @@ #include "base/gtest_prod_util.h" #include "base/ref_counted.h" #include "base/scoped_ptr.h" +#include "base/values.h" #include "chrome/browser/chrome_thread.h" #include "chrome/browser/prefs/pref_notifier.h" #include "chrome/common/pref_store.h" @@ -21,7 +23,6 @@ class FilePath; class PrefStore; class Profile; -class Value; // The PrefValueStore manages various sources of values for Preferences // (e.g., configuration policies, extensions, and user settings). It returns @@ -48,12 +49,20 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> { ~PrefValueStore(); - // Get the preference value for the given preference name. - // Return true if a value for the given preference name was found in any of - // the available PrefStores. Most callers should use Preference::GetValue() - // instead of calling this method directly. + // Gets the value for the given preference name that has a valid value type; + // that is, the same type the preference was registered with, or NULL for + // default values of Dictionaries and Lists. Returns true if a valid value + // was found in any of the available PrefStores. Most callers should use + // Preference::GetValue() instead of calling this method directly. bool GetValue(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); + + // Gets the registered value type for the given preference name. Returns + // Value::TYPE_NULL if the preference has never been registered. + Value::ValueType GetRegisteredType(const std::string& name) const; + // Read preference values into the three PrefStores so that they are available // through the GetValue method. Return the first error that occurs (but // continue reading the remaining PrefStores). @@ -68,7 +77,8 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> { // the user prefs are expected to be written out. void ScheduleWritePrefs(); - // Returns true if the PrefValueStore contains the given preference. + // Returns true if the PrefValueStore contains the given preference with a + // non-default value set with the applicable (registered) type. bool HasPrefPath(const char* name) const; // Called by the PrefNotifier when the value of the preference at |path| has @@ -175,8 +185,15 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> { scoped_ptr<PrefStore> pref_stores_[PrefNotifier::PREF_STORE_TYPE_MAX + 1]; + // A mapping of preference names to their registered types. + typedef std::map<std::string, Value::ValueType> PrefTypeMap; + PrefTypeMap pref_types_; + + // Returns true if the preference with the given name has a value in the + // given PrefStoreType, of the same value type as the preference was + // registered with. bool PrefValueInStore(const char* name, - PrefNotifier::PrefStoreType type) const; + PrefNotifier::PrefStoreType store) const; // Called during policy refresh after ReadPrefs completes on the thread // that initiated the policy refresh. RefreshPolicyPrefsCompletion takes diff --git a/chrome/browser/prefs/pref_value_store_unittest.cc b/chrome/browser/prefs/pref_value_store_unittest.cc index 0719153..fcb5b60 100644 --- a/chrome/browser/prefs/pref_value_store_unittest.cc +++ b/chrome/browser/prefs/pref_value_store_unittest.cc @@ -43,16 +43,6 @@ namespace prefs { } // Potentially expected values of all preferences used in this test program. -// The "user" namespace is defined globally in an ARM system header, so we need -// something different here. -namespace user_pref { - const int kMaxTabsValue = 31; - const bool kDeleteCacheValue = true; - const char kCurrentThemeIDValue[] = "abcdefg"; - const char kHomepageValue[] = "http://www.google.com"; - const char kApplicationLocaleValue[] = "is-WRONG"; -} - namespace enforced_pref { const std::string kHomepageValue = "http://www.topeka.com"; } @@ -68,6 +58,16 @@ namespace command_line_pref { const char kHomepageValue[] = "http://www.ferretcentral.org"; } +// The "user" namespace is defined globally in an ARM system header, so we need +// something different here. +namespace user_pref { + const int kMaxTabsValue = 31; + const bool kDeleteCacheValue = true; + const char kCurrentThemeIDValue[] = "abcdefg"; + const char kHomepageValue[] = "http://www.google.com"; + const char kApplicationLocaleValue[] = "is-WRONG"; +} + namespace recommended_pref { const int kMaxTabsValue = 10; const bool kRecommendedPrefValue = true; @@ -75,6 +75,7 @@ namespace recommended_pref { namespace default_pref { const int kDefaultValue = 7; + const char kHomepageValue[] = "default homepage"; } class PrefValueStoreTest : public testing::Test { @@ -112,6 +113,26 @@ class PrefValueStoreTest : public testing::Test { recommended_pref_store_, default_pref_store_); + // Register prefs with the PrefValueStore. + pref_value_store_->RegisterPreferenceType(prefs::kApplicationLocale, + Value::TYPE_STRING); + pref_value_store_->RegisterPreferenceType(prefs::kCurrentThemeID, + Value::TYPE_STRING); + pref_value_store_->RegisterPreferenceType(prefs::kDeleteCache, + Value::TYPE_BOOLEAN); + pref_value_store_->RegisterPreferenceType(prefs::kHomepage, + Value::TYPE_STRING); + pref_value_store_->RegisterPreferenceType(prefs::kMaxTabs, + Value::TYPE_INTEGER); + pref_value_store_->RegisterPreferenceType(prefs::kRecommendedPref, + Value::TYPE_BOOLEAN); + pref_value_store_->RegisterPreferenceType(prefs::kSampleDict, + Value::TYPE_DICTIONARY); + pref_value_store_->RegisterPreferenceType(prefs::kSampleList, + Value::TYPE_LIST); + pref_value_store_->RegisterPreferenceType(prefs::kDefaultPref, + Value::TYPE_INTEGER); + ui_thread_.reset(new ChromeThread(ChromeThread::UI, &loop_)); file_thread_.reset(new ChromeThread(ChromeThread::FILE, &loop_)); } @@ -306,6 +327,37 @@ TEST_F(PrefValueStoreTest, GetValue) { ASSERT_TRUE(v_null == NULL); } +// Make sure that if a preference changes type, so the wrong type is stored in +// the user pref file, it uses the correct fallback value instead. +TEST_F(PrefValueStoreTest, GetValueChangedType) { + // Check falling back to a recommended value. + user_pref_store_->prefs()->SetString(prefs::kMaxTabs, "not an integer"); + Value* value = NULL; + ASSERT_TRUE(pref_value_store_->GetValue(prefs::kMaxTabs, &value)); + ASSERT_TRUE(value != NULL); + ASSERT_EQ(Value::TYPE_INTEGER, value->GetType()); + int actual_int_value = -1; + EXPECT_TRUE(value->GetAsInteger(&actual_int_value)); + EXPECT_EQ(recommended_pref::kMaxTabsValue, actual_int_value); + + // Check falling back multiple times, to a default string. + enforced_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1); + extension_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1); + command_line_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1); + user_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1); + recommended_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1); + default_pref_store_->prefs()->SetString(prefs::kHomepage, + default_pref::kHomepageValue); + + value = NULL; + ASSERT_TRUE(pref_value_store_->GetValue(prefs::kHomepage, &value)); + ASSERT_TRUE(value != NULL); + ASSERT_EQ(Value::TYPE_STRING, value->GetType()); + std::string actual_str_value; + EXPECT_TRUE(value->GetAsString(&actual_str_value)); + EXPECT_EQ(default_pref::kHomepageValue, actual_str_value); +} + TEST_F(PrefValueStoreTest, HasPrefPath) { // Enforced preference EXPECT_TRUE(pref_value_store_->HasPrefPath(prefs::kHomepage)); @@ -322,10 +374,15 @@ TEST_F(PrefValueStoreTest, HasPrefPath) { TEST_F(PrefValueStoreTest, PrefHasChanged) { // Setup. const char managed_pref_path[] = "managed_pref"; + pref_value_store_->RegisterPreferenceType(managed_pref_path, + Value::TYPE_STRING); enforced_pref_store_->prefs()->SetString(managed_pref_path, "managed value"); const char user_pref_path[] = "user_pref"; + pref_value_store_->RegisterPreferenceType(user_pref_path, Value::TYPE_STRING); user_pref_store_->prefs()->SetString(user_pref_path, "user value"); const char default_pref_path[] = "default_pref"; + pref_value_store_->RegisterPreferenceType(default_pref_path, + Value::TYPE_STRING); default_pref_store_->prefs()->SetString(default_pref_path, "default value"); // Check pref controlled by highest-priority store. @@ -399,7 +456,7 @@ TEST_F(PrefValueStoreTest, SetUserPrefValue) { actual_value = NULL; std::string key(prefs::kSampleDict); - pref_value_store_->GetValue(key , &actual_value); + pref_value_store_->GetValue(key, &actual_value); ASSERT_EQ(expected_dict_value, actual_value); ASSERT_TRUE(expected_dict_value->Equals(actual_value)); diff --git a/chrome/browser/sync/glue/preference_model_associator_unittest.cc b/chrome/browser/sync/glue/preference_model_associator_unittest.cc index 2939db4..b3dca08 100644 --- a/chrome/browser/sync/glue/preference_model_associator_unittest.cc +++ b/chrome/browser/sync/glue/preference_model_associator_unittest.cc @@ -44,7 +44,7 @@ class AbstractPreferenceMergeTest : public testing::Test { const PrefService::Preference* pref = pref_service_->FindPreference(pref_name.c_str()); ASSERT_TRUE(pref); - Value::ValueType type = pref->type(); + Value::ValueType type = pref->GetType(); if (type == Value::TYPE_DICTIONARY) empty_value.reset(new DictionaryValue); else if (type == Value::TYPE_LIST) |