diff options
Diffstat (limited to 'chrome/browser')
| -rw-r--r-- | chrome/browser/extensions/extension_pref_store.cc | 2 | ||||
| -rw-r--r-- | chrome/browser/extensions/extension_pref_store_unittest.cc | 60 | ||||
| -rw-r--r-- | chrome/browser/pref_notifier.cc | 140 | ||||
| -rw-r--r-- | chrome/browser/pref_notifier.h | 108 | ||||
| -rw-r--r-- | chrome/browser/pref_notifier_unittest.cc | 282 | ||||
| -rw-r--r-- | chrome/browser/pref_service.cc | 144 | ||||
| -rw-r--r-- | chrome/browser/pref_service.h | 69 | ||||
| -rw-r--r-- | chrome/browser/pref_value_store.cc | 74 | ||||
| -rw-r--r-- | chrome/browser/pref_value_store.h | 44 | ||||
| -rw-r--r-- | chrome/browser/pref_value_store_unittest.cc | 37 | ||||
| -rw-r--r-- | chrome/browser/scoped_pref_update.cc | 2 |
11 files changed, 687 insertions, 275 deletions
diff --git a/chrome/browser/extensions/extension_pref_store.cc b/chrome/browser/extensions/extension_pref_store.cc index dbac8cc..c8f4b91 100644 --- a/chrome/browser/extensions/extension_pref_store.cc +++ b/chrome/browser/extensions/extension_pref_store.cc @@ -115,7 +115,7 @@ void ExtensionPrefStore::UpdateOnePref(const char* path) { } if (pref_service) - pref_service->FireObserversIfChanged(path, old_value.get()); + pref_service->pref_notifier()->OnPreferenceSet(path, old_value.get()); } void ExtensionPrefStore::UpdatePrefs(const PrefValueMap* pref_values) { diff --git a/chrome/browser/extensions/extension_pref_store_unittest.cc b/chrome/browser/extensions/extension_pref_store_unittest.cc index 05cd322..a05730a 100644 --- a/chrome/browser/extensions/extension_pref_store_unittest.cc +++ b/chrome/browser/extensions/extension_pref_store_unittest.cc @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include <gtest/gtest.h> - #include <string> #include <vector> @@ -15,6 +13,8 @@ #include "chrome/browser/pref_value_store.h" #include "chrome/common/extensions/extension.h" #include "chrome/test/testing_pref_service.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" namespace { @@ -76,26 +76,34 @@ class TestExtensionPrefStore : public ExtensionPrefStore { PrefService* pref_service_; }; +// Mock PrefNotifier that allows the notifications to be tracked. +class MockPrefNotifier : public PrefNotifier { + public: + MockPrefNotifier(PrefService* service, PrefValueStore* value_store) + : PrefNotifier(service, value_store) {} + + virtual ~MockPrefNotifier() {} + + MOCK_METHOD1(FireObservers, void(const char* path)); +}; + +// Mock PrefService that allows the PrefNotifier to be injected. class MockPrefService : public PrefService { public: - explicit MockPrefService(PrefValueStore* value_store) - : PrefService(value_store), - fired_observers_(false) {} - - // Tracks whether the observers would have been notified. - virtual void FireObserversIfChanged(const char* pref_name, - const Value* old_value) { - fired_observers_ = PrefIsChanged(pref_name, old_value); + explicit MockPrefService(PrefValueStore* pref_value_store) + : PrefService(pref_value_store) { } - bool fired_observers_; + void SetPrefNotifier(MockPrefNotifier* notifier) { + pref_notifier_.reset(notifier); + } }; -// Use static constants to avoid confusing std::map with hard-coded strings. -static const char* kPref1 = "path1.subpath"; -static const char* kPref2 = "path2"; -static const char* kPref3 = "path3"; -static const char* kPref4 = "path4"; +// Use constants to avoid confusing std::map with hard-coded strings. +const char kPref1[] = "path1.subpath"; +const char kPref2[] = "path2"; +const char kPref3[] = "path3"; +const char kPref4[] = "path4"; } // namespace @@ -316,28 +324,36 @@ TEST(ExtensionPrefStoreTest, UninstallExtensionFromMiddle) { } TEST(ExtensionPrefStoreTest, NotifyWhenNeeded) { + using testing::Mock; + TestExtensionPrefStore* eps = new TestExtensionPrefStore; ASSERT_TRUE(eps->ext1 != NULL); // The PrefValueStore takes ownership of the PrefStores; in this case, that's // only an ExtensionPrefStore. Likewise, the PrefService takes ownership of - // the PrefValueStore. + // the PrefValueStore and PrefNotifier. PrefValueStore* value_store = new TestingPrefService::TestingPrefValueStore( NULL, eps, NULL, NULL, NULL); scoped_ptr<MockPrefService> pref_service(new MockPrefService(value_store)); + MockPrefNotifier* pref_notifier = new MockPrefNotifier(pref_service.get(), + value_store); + pref_service->SetPrefNotifier(pref_notifier); + eps->SetPrefService(pref_service.get()); pref_service->RegisterStringPref(kPref1, std::string()); + EXPECT_CALL(*pref_notifier, FireObservers(kPref1)); eps->InstallExtensionPref(eps->ext1, kPref1, Value::CreateStringValue("https://www.chromium.org")); - EXPECT_TRUE(pref_service->fired_observers_); + Mock::VerifyAndClearExpectations(pref_notifier); + + EXPECT_CALL(*pref_notifier, FireObservers(kPref1)).Times(0); eps->InstallExtensionPref(eps->ext1, kPref1, Value::CreateStringValue("https://www.chromium.org")); - EXPECT_FALSE(pref_service->fired_observers_); + Mock::VerifyAndClearExpectations(pref_notifier); + + EXPECT_CALL(*pref_notifier, FireObservers(kPref1)).Times(2); eps->InstallExtensionPref(eps->ext1, kPref1, Value::CreateStringValue("chrome://newtab")); - EXPECT_TRUE(pref_service->fired_observers_); - eps->UninstallExtension(eps->ext1); - EXPECT_TRUE(pref_service->fired_observers_); } diff --git a/chrome/browser/pref_notifier.cc b/chrome/browser/pref_notifier.cc new file mode 100644 index 0000000..8cf5f1d --- /dev/null +++ b/chrome/browser/pref_notifier.cc @@ -0,0 +1,140 @@ +// Copyright (c) 2010 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/pref_notifier.h" + +#include "base/stl_util-inl.h" +#include "base/utf_string_conversions.h" +#include "chrome/browser/policy/configuration_policy_pref_store.h" +#include "chrome/browser/pref_service.h" +#include "chrome/browser/pref_value_store.h" +#include "chrome/common/notification_service.h" + + +PrefNotifier::PrefNotifier(PrefService* service, PrefValueStore* value_store) + : pref_service_(service), + pref_value_store_(value_store) { + registrar_.Add(this, + NotificationType(NotificationType::POLICY_CHANGED), + NotificationService::AllSources()); +} + +PrefNotifier::~PrefNotifier() { + DCHECK(CalledOnValidThread()); + + // Verify that there are no pref observers when we shut down. + for (PrefObserverMap::iterator it = pref_observers_.begin(); + it != pref_observers_.end(); ++it) { + NotificationObserverList::Iterator obs_iterator(*(it->second)); + if (obs_iterator.GetNext()) { + LOG(WARNING) << "pref observer found at shutdown " << it->first; + } + } + + STLDeleteContainerPairSecondPointers(pref_observers_.begin(), + pref_observers_.end()); + pref_observers_.clear(); +} + +void PrefNotifier::OnPreferenceSet(const char* pref_name, + const Value* old_value) { + if (pref_value_store_->PrefHasChanged(pref_name, old_value)) + FireObservers(pref_name); +} + +void PrefNotifier::OnUserPreferenceSet(const char* pref_name, + const Value* old_value) { + // TODO(pamg): Adjust to account for source PrefStore. + OnPreferenceSet(pref_name, old_value); +} + +void PrefNotifier::FireObservers(const char* path) { + DCHECK(CalledOnValidThread()); + + // Convert path to a std::string because the Details constructor requires a + // class. + std::string path_str(path); + PrefObserverMap::iterator observer_iterator = pref_observers_.find(path_str); + if (observer_iterator == pref_observers_.end()) + return; + + NotificationObserverList::Iterator it(*(observer_iterator->second)); + NotificationObserver* observer; + while ((observer = it.GetNext()) != NULL) { + observer->Observe(NotificationType::PREF_CHANGED, + Source<PrefService>(pref_service_), + Details<std::string>(&path_str)); + } +} + +void PrefNotifier::AddPrefObserver(const char* path, + NotificationObserver* obs) { + // Get the pref observer list associated with the path. + NotificationObserverList* observer_list = NULL; + PrefObserverMap::iterator observer_iterator = pref_observers_.find(path); + if (observer_iterator == pref_observers_.end()) { + observer_list = new NotificationObserverList; + pref_observers_[path] = observer_list; + } else { + observer_list = observer_iterator->second; + } + + // Verify that this observer doesn't already exist. + NotificationObserverList::Iterator it(*observer_list); + NotificationObserver* existing_obs; + while ((existing_obs = it.GetNext()) != NULL) { + DCHECK(existing_obs != obs) << path << " observer already registered"; + if (existing_obs == obs) + return; + } + + // Ok, safe to add the pref observer. + observer_list->AddObserver(obs); +} + +void PrefNotifier::RemovePrefObserver(const char* path, + NotificationObserver* obs) { + DCHECK(CalledOnValidThread()); + + PrefObserverMap::iterator observer_iterator = pref_observers_.find(path); + if (observer_iterator == pref_observers_.end()) { + return; + } + + NotificationObserverList* observer_list = observer_iterator->second; + observer_list->RemoveObserver(obs); +} + +void PrefNotifier::FireObserversForRefreshedManagedPrefs( + std::vector<std::string> changed_prefs_paths) { + DCHECK(CalledOnValidThread()); + std::vector<std::string>::const_iterator current; + for (current = changed_prefs_paths.begin(); + current != changed_prefs_paths.end(); + ++current) { + FireObservers(current->c_str()); + } +} + +void PrefNotifier::Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details) { + if (type == NotificationType::POLICY_CHANGED) { + PrefValueStore::AfterRefreshCallback* callback = + NewCallback(this, + &PrefNotifier::FireObserversForRefreshedManagedPrefs); + // The notification of the policy refresh can come from any thread, + // but the manipulation of the PrefValueStore must happen on the UI + // thread, thus the policy refresh must be explicitly started on it. + ChromeThread::PostTask( + ChromeThread::UI, FROM_HERE, + NewRunnableMethod( + pref_value_store_, + &PrefValueStore::RefreshPolicyPrefs, + ConfigurationPolicyPrefStore::CreateManagedPolicyPrefStore(), + ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore(), + + callback)); + } +} diff --git a/chrome/browser/pref_notifier.h b/chrome/browser/pref_notifier.h new file mode 100644 index 0000000..3779818 --- /dev/null +++ b/chrome/browser/pref_notifier.h @@ -0,0 +1,108 @@ +// Copyright (c) 2010 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. + +#ifndef CHROME_BROWSER_PREF_NOTIFIER_H_ +#define CHROME_BROWSER_PREF_NOTIFIER_H_ +#pragma once + +#include <string> +#include <vector> + +#include "base/hash_tables.h" +#include "base/non_thread_safe.h" +#include "base/observer_list.h" +#include "chrome/common/notification_registrar.h" + +class NotificationObserver; +class PrefService; +class PrefValueStore; +class Value; + +// Registers observers for particular preferences and sends notifications when +// preference values or sources (i.e., which preference layer controls the +// preference) change. +class PrefNotifier : public NonThreadSafe, + public NotificationObserver { + public: + // PrefStores must be listed here in order from highest to lowest priority. + // MANAGED contains all managed (policy) preference values. + // EXTENSION contains preference values set by extensions. + // COMMAND_LINE contains preference values set by command-line switches. + // USER contains all user-set preference values. + // RECOMMENDED contains all recommended (policy) preference values. + // This enum is kept in pref_notifier.h rather than pref_value_store.h in + // order to minimize additional headers needed by the *PrefStore files. + enum PrefStoreType { + // INVALID_STORE is not associated with an actual PrefStore but used as + // an invalid marker, e.g. as a return value. + INVALID_STORE = -1, + MANAGED_STORE = 0, + EXTENSION_STORE, + COMMAND_LINE_STORE, + USER_STORE, + RECOMMENDED_STORE, + PREF_STORE_TYPE_MAX = RECOMMENDED_STORE + }; + + // The |service| with which this notifier is associated will be sent as the + // source of any notifications. + PrefNotifier(PrefService* service, PrefValueStore* value_store); + + virtual ~PrefNotifier(); + + // For the given pref_name, fire any observer of the pref only if |old_value| + // is different from the current value. + // TODO(pamg): Also send notifications if the controlling store has changed. + void OnPreferenceSet(const char* pref_name, const Value* old_value); + + // For the given pref_name, fire any observer of the pref only if |old_value| + // is different from the current value. Convenience method to be called when + // a preference is set in the USER_STORE. + void OnUserPreferenceSet(const char* pref_name, const Value* old_value); + + // For the given pref_name, fire any observer of the pref. Virtual so it can + // be mocked for unit testing. + virtual void FireObservers(const char* path); + + // If the pref at the given path changes, we call the observer's Observe + // method with NOTIFY_PREF_CHANGED. + void AddPrefObserver(const char* path, NotificationObserver* obs); + void RemovePrefObserver(const char* path, NotificationObserver* obs); + + protected: + // A map from pref names to a list of observers. Observers get fired in the + // order they are added. These should only be accessed externally for unit + // testing. + typedef ObserverList<NotificationObserver> NotificationObserverList; + typedef base::hash_map<std::string, NotificationObserverList*> + PrefObserverMap; + const PrefObserverMap* pref_observers() { return &pref_observers_; } + + private: + // Weak references. + PrefService* pref_service_; + PrefValueStore* pref_value_store_; + + NotificationRegistrar registrar_; + + PrefObserverMap pref_observers_; + + // Called after a policy refresh to notify relevant preference observers. + // |changed_prefs_paths| is the vector of preference paths changed by the + // policy update. It is passed by value and not reference because + // this method is called asynchronously as a callback from another thread. + // Copying the vector guarantees that the vector's lifecycle spans the + // method's invocation. + void FireObserversForRefreshedManagedPrefs( + std::vector<std::string> changed_prefs_paths); + + // NotificationObserver methods: + virtual void Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details); + + DISALLOW_COPY_AND_ASSIGN(PrefNotifier); +}; + +#endif // CHROME_BROWSER_PREF_NOTIFIER_H_ diff --git a/chrome/browser/pref_notifier_unittest.cc b/chrome/browser/pref_notifier_unittest.cc new file mode 100644 index 0000000..a6c2da9 --- /dev/null +++ b/chrome/browser/pref_notifier_unittest.cc @@ -0,0 +1,282 @@ +// Copyright (c) 2010 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/pref_notifier.h" +#include "chrome/browser/pref_service.h" +#include "chrome/browser/pref_value_store.h" +#include "chrome/common/notification_observer.h" +#include "chrome/common/notification_type.h" +#include "chrome/common/notification_service.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + + +namespace { + +const char kChangedPref[] = "changed_pref"; +const char kUnchangedPref[] = "unchanged_pref"; + +bool DetailsAreChangedPref(const Details<std::string>& details) { + std::string* string_in = Details<std::string>(details).ptr(); + return strcmp(string_in->c_str(), kChangedPref) == 0; +} + +// Test PrefNotifier that allows tracking of observers and notifications. +class MockPrefNotifier : public PrefNotifier { + public: + MockPrefNotifier(PrefService* prefs, PrefValueStore* value_store) + : PrefNotifier(prefs, value_store) {} + + virtual ~MockPrefNotifier() {} + + MOCK_METHOD1(FireObservers, void(const char* path)); + + void RealFireObservers(const char* path) { + PrefNotifier::FireObservers(path); + } + + size_t CountObserver(const char* path, NotificationObserver* obs) { + PrefObserverMap::const_iterator observer_iterator = + pref_observers()->find(path); + if (observer_iterator == pref_observers()->end()) + return false; + + NotificationObserverList* observer_list = observer_iterator->second; + NotificationObserverList::Iterator it(*observer_list); + NotificationObserver* existing_obs; + size_t count = 0; + while ((existing_obs = it.GetNext()) != NULL) { + if (existing_obs == obs) + count++; + } + + return count; + } +}; + +// Mock PrefValueStore that has no PrefStores and injects a simpler +// PrefHasChanged(). +class MockPrefValueStore : public PrefValueStore { + public: + MockPrefValueStore() : PrefValueStore(NULL, NULL, NULL, NULL, NULL) {} + + virtual ~MockPrefValueStore() {} + + // This mock version returns true if the pref path starts with "changed". + virtual bool PrefHasChanged(const char* path, const Value* old_value) { + std::string path_str(path); + if (path_str.compare(0, 7, "changed") == 0) + return true; + return false; + } +}; + +// Mock PrefService that allows the PrefNotifier to be injected. +class MockPrefService : public PrefService { + public: + explicit MockPrefService(PrefValueStore* pref_value_store) + : PrefService(pref_value_store) {} + + void SetPrefNotifier(PrefNotifier* notifier) { + pref_notifier_.reset(notifier); + } +}; + +// Mock PrefObserver that verifies notifications. +class MockPrefObserver : public NotificationObserver { + public: + virtual ~MockPrefObserver() {} + + MOCK_METHOD3(Observe, void(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details)); +}; + +// Test fixture class. +class PrefNotifierTest : public testing::Test { + protected: + virtual void SetUp() { + value_store_ = new MockPrefValueStore; + pref_service_.reset(new MockPrefService(value_store_)); + notifier_ = new MockPrefNotifier(pref_service_.get(), value_store_); + pref_service_->SetPrefNotifier(notifier_); + } + + // The PrefService takes ownership of the PrefValueStore and PrefNotifier. + PrefValueStore* value_store_; + MockPrefNotifier* notifier_; + scoped_ptr<MockPrefService> pref_service_; + + MockPrefObserver obs1_; + MockPrefObserver obs2_; +}; + + +TEST_F(PrefNotifierTest, OnPreferenceSet) { + EXPECT_CALL(*notifier_, FireObservers(kChangedPref)); + EXPECT_CALL(*notifier_, FireObservers(kUnchangedPref)).Times(0); + notifier_->OnPreferenceSet(kChangedPref, NULL); + notifier_->OnPreferenceSet(kUnchangedPref, NULL); +} + +TEST_F(PrefNotifierTest, OnUserPreferenceSet) { + EXPECT_CALL(*notifier_, FireObservers(kChangedPref)); + EXPECT_CALL(*notifier_, FireObservers(kUnchangedPref)).Times(0); + notifier_->OnUserPreferenceSet(kChangedPref, NULL); + notifier_->OnUserPreferenceSet(kUnchangedPref, NULL); +} + +TEST_F(PrefNotifierTest, AddAndRemovePrefObservers) { + const char pref_name[] = "homepage"; + const char pref_name2[] = "proxy"; + + notifier_->AddPrefObserver(pref_name, &obs1_); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); + + // Re-adding the same observer for the same pref doesn't change anything. + // Skip this in debug mode, since it hits a DCHECK and death tests aren't + // thread-safe. +#if defined(NDEBUG) + notifier_->AddPrefObserver(pref_name, &obs1_); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); +#endif // NDEBUG + + // Ensure that we can add the same observer to a different pref. + notifier_->AddPrefObserver(pref_name2, &obs1_); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); + + // Ensure that we can add another observer to the same pref. + notifier_->AddPrefObserver(pref_name, &obs2_); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); + + // Ensure that we can remove all observers, and that removing a non-existent + // observer is harmless. + notifier_->RemovePrefObserver(pref_name, &obs1_); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); + + notifier_->RemovePrefObserver(pref_name, &obs2_); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); + + notifier_->RemovePrefObserver(pref_name, &obs1_); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(1u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); + + notifier_->RemovePrefObserver(pref_name2, &obs1_); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs1_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name, &obs2_)); + ASSERT_EQ(0u, notifier_->CountObserver(pref_name2, &obs2_)); +} + +TEST_F(PrefNotifierTest, FireObservers) { + using testing::_; + using testing::Field; + using testing::Invoke; + using testing::Mock; + using testing::Truly; + + // Tell googlemock to pass calls to the mock's "back door" too. + ON_CALL(*notifier_, FireObservers(_)) + .WillByDefault(Invoke(notifier_, &MockPrefNotifier::RealFireObservers)); + EXPECT_CALL(*notifier_, FireObservers(kChangedPref)).Times(4); + EXPECT_CALL(*notifier_, FireObservers(kUnchangedPref)).Times(0); + + notifier_->AddPrefObserver(kChangedPref, &obs1_); + notifier_->AddPrefObserver(kUnchangedPref, &obs1_); + + EXPECT_CALL(obs1_, Observe( + Field(&NotificationType::value, NotificationType::PREF_CHANGED), + Source<PrefService>(pref_service_.get()), + Truly(DetailsAreChangedPref))); + EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0); + notifier_->OnPreferenceSet(kChangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0); + EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0); + notifier_->OnPreferenceSet(kUnchangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + notifier_->AddPrefObserver(kChangedPref, &obs2_); + notifier_->AddPrefObserver(kUnchangedPref, &obs2_); + + EXPECT_CALL(obs1_, Observe( + Field(&NotificationType::value, NotificationType::PREF_CHANGED), + Source<PrefService>(pref_service_.get()), + Truly(DetailsAreChangedPref))); + EXPECT_CALL(obs2_, Observe( + Field(&NotificationType::value, NotificationType::PREF_CHANGED), + Source<PrefService>(pref_service_.get()), + Truly(DetailsAreChangedPref))); + notifier_->OnPreferenceSet(kChangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0); + EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0); + notifier_->OnPreferenceSet(kUnchangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + // Make sure removing an observer from one pref doesn't affect anything else. + notifier_->RemovePrefObserver(kChangedPref, &obs1_); + + EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0); + EXPECT_CALL(obs2_, Observe( + Field(&NotificationType::value, NotificationType::PREF_CHANGED), + Source<PrefService>(pref_service_.get()), + Truly(DetailsAreChangedPref))); + notifier_->OnPreferenceSet(kChangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0); + EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0); + notifier_->OnPreferenceSet(kUnchangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + // Make sure removing an observer entirely doesn't affect anything else. + notifier_->RemovePrefObserver(kUnchangedPref, &obs1_); + + EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0); + EXPECT_CALL(obs2_, Observe( + Field(&NotificationType::value, NotificationType::PREF_CHANGED), + Source<PrefService>(pref_service_.get()), + Truly(DetailsAreChangedPref))); + notifier_->OnPreferenceSet(kChangedPref, NULL); + Mock::VerifyAndClearExpectations(&obs1_); + Mock::VerifyAndClearExpectations(&obs2_); + + EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0); + EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0); + notifier_->OnPreferenceSet(kUnchangedPref, NULL); + + notifier_->RemovePrefObserver(kChangedPref, &obs2_); + notifier_->RemovePrefObserver(kUnchangedPref, &obs2_); +} + +} // namespace diff --git a/chrome/browser/pref_service.cc b/chrome/browser/pref_service.cc index 9e3ad7e..aff5f2e 100644 --- a/chrome/browser/pref_service.cc +++ b/chrome/browser/pref_service.cc @@ -20,7 +20,6 @@ #include "base/utf_string_conversions.h" #include "build/build_config.h" #include "chrome/browser/chrome_thread.h" -#include "chrome/browser/policy/configuration_policy_pref_store.h" #include "chrome/browser/profile.h" #include "chrome/common/notification_service.h" #include "grit/chromium_strings.h" @@ -93,29 +92,14 @@ PrefService* PrefService::CreateUserPrefService(const FilePath& pref_filename) { PrefService::PrefService(PrefValueStore* pref_value_store) : pref_value_store_(pref_value_store) { + pref_notifier_.reset(new PrefNotifier(this, pref_value_store)); InitFromStorage(); - registrar_.Add(this, - NotificationType(NotificationType::POLICY_CHANGED), - NotificationService::AllSources()); } PrefService::~PrefService() { DCHECK(CalledOnValidThread()); - - // Verify that there are no pref observers when we shut down. - for (PrefObserverMap::iterator it = pref_observers_.begin(); - it != pref_observers_.end(); ++it) { - NotificationObserverList::Iterator obs_iterator(*(it->second)); - if (obs_iterator.GetNext()) { - LOG(WARNING) << "pref observer found at shutdown " << it->first; - } - } - STLDeleteContainerPointers(prefs_.begin(), prefs_.end()); prefs_.clear(); - STLDeleteContainerPairSecondPointers(pref_observers_.begin(), - pref_observers_.end()); - pref_observers_.clear(); } void PrefService::InitFromStorage() { @@ -341,39 +325,6 @@ bool PrefService::IsManagedPreference(const char* pref_name) const { return false; } -void PrefService::FireObserversIfChanged(const char* path, - const Value* old_value) { - if (PrefIsChanged(path, old_value)) - FireObservers(path); -} - -void PrefService::FireObservers(const char* path) { - DCHECK(CalledOnValidThread()); - - // Convert path to a std::string because the Details constructor requires a - // class. - std::string path_str(path); - PrefObserverMap::iterator observer_iterator = pref_observers_.find(path_str); - if (observer_iterator == pref_observers_.end()) - return; - - NotificationObserverList::Iterator it(*(observer_iterator->second)); - NotificationObserver* observer; - while ((observer = it.GetNext()) != NULL) { - observer->Observe(NotificationType::PREF_CHANGED, - Source<PrefService>(this), - Details<std::string>(&path_str)); - } -} - -bool PrefService::PrefIsChanged(const char* path, - const Value* old_value) { - Value* new_value = NULL; - pref_value_store_->GetValue(path, &new_value); - // Some unit tests have no values for certain prefs. - return (!new_value || !old_value->Equals(new_value)); -} - const DictionaryValue* PrefService::GetDictionary(const char* path) const { DCHECK(CalledOnValidThread()); @@ -404,49 +355,12 @@ const ListValue* PrefService::GetList(const char* path) const { void PrefService::AddPrefObserver(const char* path, NotificationObserver* obs) { - DCHECK(CalledOnValidThread()); - - const Preference* pref = FindPreference(path); - if (!pref) { - NOTREACHED() << "Trying to add an observer for an unregistered pref: " - << path; - return; - } - - // Get the pref observer list associated with the path. - NotificationObserverList* observer_list = NULL; - PrefObserverMap::iterator observer_iterator = pref_observers_.find(path); - if (observer_iterator == pref_observers_.end()) { - observer_list = new NotificationObserverList; - pref_observers_[path] = observer_list; - } else { - observer_list = observer_iterator->second; - } - - // Verify that this observer doesn't already exist. - NotificationObserverList::Iterator it(*observer_list); - NotificationObserver* existing_obs; - while ((existing_obs = it.GetNext()) != NULL) { - DCHECK(existing_obs != obs) << path << " observer already registered"; - if (existing_obs == obs) - return; - } - - // Ok, safe to add the pref observer. - observer_list->AddObserver(obs); + pref_notifier_->AddPrefObserver(path, obs); } void PrefService::RemovePrefObserver(const char* path, NotificationObserver* obs) { - DCHECK(CalledOnValidThread()); - - PrefObserverMap::iterator observer_iterator = pref_observers_.find(path); - if (observer_iterator == pref_observers_.end()) { - return; - } - - NotificationObserverList* observer_list = observer_iterator->second; - observer_list->RemoveObserver(obs); + pref_notifier_->RemovePrefObserver(path, obs); } void PrefService::RegisterPreference(Preference* pref) { @@ -472,8 +386,10 @@ void PrefService::ClearPref(const char* path) { bool has_old_value = pref_value_store_->GetValue(path, &value); pref_value_store_->RemoveUserPrefValue(path); + // TODO(pamg): Removing the user value when there's a higher-priority setting + // doesn't change the value and shouldn't fire observers. if (has_old_value) - FireObservers(path); + pref_notifier_->FireObservers(path); } void PrefService::Set(const char* path, const Value& value) { @@ -492,7 +408,7 @@ void PrefService::Set(const char* path, const Value& value) { scoped_ptr<Value> old_value(GetPrefCopy(path)); if (!old_value->Equals(&value)) { pref_value_store_->RemoveUserPrefValue(path); - FireObservers(path); + pref_notifier_->FireObservers(path); } return; } @@ -504,7 +420,7 @@ void PrefService::Set(const char* path, const Value& value) { scoped_ptr<Value> old_value(GetPrefCopy(path)); pref_value_store_->SetUserPrefValue(path, value.DeepCopy()); - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } void PrefService::SetBoolean(const char* path, bool value) { @@ -528,7 +444,7 @@ void PrefService::SetBoolean(const char* path, bool value) { Value* new_value = Value::CreateBooleanValue(value); pref_value_store_->SetUserPrefValue(path, new_value); - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } void PrefService::SetInteger(const char* path, int value) { @@ -552,7 +468,7 @@ void PrefService::SetInteger(const char* path, int value) { Value* new_value = Value::CreateIntegerValue(value); pref_value_store_->SetUserPrefValue(path, new_value); - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } void PrefService::SetReal(const char* path, double value) { @@ -576,7 +492,7 @@ void PrefService::SetReal(const char* path, double value) { Value* new_value = Value::CreateRealValue(value); pref_value_store_->SetUserPrefValue(path, new_value); - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } void PrefService::SetString(const char* path, const std::string& value) { @@ -600,7 +516,7 @@ void PrefService::SetString(const char* path, const std::string& value) { Value* new_value = Value::CreateStringValue(value); pref_value_store_->SetUserPrefValue(path, new_value); - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } void PrefService::SetFilePath(const char* path, const FilePath& value) { @@ -632,7 +548,7 @@ void PrefService::SetFilePath(const char* path, const FilePath& value) { pref_value_store_->SetUserPrefValue(path, new_value); #endif - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } void PrefService::SetInt64(const char* path, int64 value) { @@ -656,7 +572,7 @@ void PrefService::SetInt64(const char* path, int64 value) { Value* new_value = Value::CreateStringValue(base::Int64ToString(value)); pref_value_store_->SetUserPrefValue(path, new_value); - FireObserversIfChanged(path, old_value.get()); + pref_notifier_->OnUserPreferenceSet(path, old_value.get()); } int64 PrefService::GetInt64(const char* path) const { @@ -802,38 +718,6 @@ bool PrefService::Preference::IsUserControlled() const { return pref_value_store_->PrefValueFromUserStore(name_.c_str()); } -void PrefService::FireObserversForRefreshedManagedPrefs( - std::vector<std::string> changed_prefs_paths) { - DCHECK(CalledOnValidThread()); - std::vector<std::string>::const_iterator current; - for (current = changed_prefs_paths.begin(); - current != changed_prefs_paths.end(); - ++current) { - FireObservers(current->c_str()); - } -} - bool PrefService::Preference::IsUserModifiable() const { return pref_value_store_->PrefValueUserModifiable(name_.c_str()); } - -void PrefService::Observe(NotificationType type, - const NotificationSource& source, - const NotificationDetails& details) { - if (type == NotificationType::POLICY_CHANGED) { - PrefValueStore::AfterRefreshCallback* callback = - NewCallback(this, - &PrefService::FireObserversForRefreshedManagedPrefs); - // The notification of the policy refresh can come from any thread, - // but the manipulation of the PrefValueStore must happen on the UI - // thread, thus the policy refresh must be explicitly started on it. - ChromeThread::PostTask( - ChromeThread::UI, FROM_HERE, - NewRunnableMethod( - pref_value_store_.get(), - &PrefValueStore::RefreshPolicyPrefs, - ConfigurationPolicyPrefStore::CreateManagedPolicyPrefStore(), - ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore(), - callback)); - } -} diff --git a/chrome/browser/pref_service.h b/chrome/browser/pref_service.h index 53a8523..6489b32 100644 --- a/chrome/browser/pref_service.h +++ b/chrome/browser/pref_service.h @@ -13,26 +13,17 @@ #include <vector> #include "base/file_path.h" -#include "base/hash_tables.h" #include "base/non_thread_safe.h" -#include "base/observer_list.h" -#include "base/ref_counted.h" #include "base/scoped_ptr.h" #include "base/values.h" #include "chrome/browser/pref_value_store.h" -#include "chrome/common/notification_observer.h" -#include "chrome/common/notification_registrar.h" #include "chrome/common/pref_store.h" -class NotificationDetails; -class NotificationSource; -class NotificationType; -class Preference; +class NotificationObserver; +class PrefNotifier; class Profile; -class ScopedPrefUpdate; -class PrefService : public NonThreadSafe, - public NotificationObserver { +class PrefService : public NonThreadSafe { public: // A helper class to store all the information associated with a preference. class Preference { @@ -114,8 +105,8 @@ class PrefService : public NonThreadSafe, // preferences). static PrefService* CreateUserPrefService(const FilePath& pref_filename); - // This constructor is primarily used by tests. The |PrefValueStore| provides - // preference values. + // This constructor is primarily used by tests. The |pref_value_store| + // provides preference values. explicit PrefService(PrefValueStore* pref_value_store); virtual ~PrefService(); @@ -225,20 +216,15 @@ class PrefService : public NonThreadSafe, // preference is not registered. const Preference* FindPreference(const char* pref_name) const; - // For the given pref_name, fire any observer of the pref only if |old_value| - // is different from the current value. Virtual so it can be mocked for a - // unit test. - virtual void FireObserversIfChanged(const char* pref_name, - const Value* old_value); - bool read_only() const { return pref_value_store_->ReadOnly(); } - protected: - // For the given pref_name, fire any observer of the pref. - void FireObservers(const char* pref_name); + PrefNotifier* pref_notifier() const { return pref_notifier_.get(); } - // This should only be accessed by subclasses for unit-testing. - bool PrefIsChanged(const char* path, const Value* old_value); + protected: + // The PrefNotifier handles registering and notifying preference observers. + // It is created and owned by this PrefService. Subclasses may access it for + // unit testing. + scoped_ptr<PrefNotifier> pref_notifier_; private: // Add a preference to the PreferenceMap. If the pref already exists, return @@ -256,42 +242,13 @@ class PrefService : public NonThreadSafe, // This should only be called from the constructor. void InitFromStorage(); - // NotificationObserver methods: - virtual void Observe(NotificationType type, - const NotificationSource& source, - const NotificationDetails& details); - - // Called after a policy refresh to notify relevant preference observers. - // |changed_prefs_paths| is the vector of preference paths changed by the - // policy update. It is passed by value and not reference because - // this method is called asynchronously as a callback from another thread. - // Copying the vector guarantees that the vector's lifecycle spans the - // method's invocation. - void FireObserversForRefreshedManagedPrefs( - std::vector<std::string> changed_prefs_paths); - - // The value of a Preference can be: - // managed, user defined, recommended or default. - // The PrefValueStore manages enforced, user defined and recommended values - // for Preferences. It returns the value of a Preference with the - // highest priority, and allows to set user defined values for preferences - // that are not managed. + // The PrefValueStore provides prioritized preference values. It is created + // and owned by this PrefService. Subclasses may access it for unit testing. scoped_refptr<PrefValueStore> pref_value_store_; - NotificationRegistrar registrar_; - // A set of all the registered Preference objects. PreferenceSet prefs_; - // A map from pref names to a list of observers. Observers get fired in the - // order they are added. - typedef ObserverList<NotificationObserver> NotificationObserverList; - typedef base::hash_map<std::string, NotificationObserverList*> - PrefObserverMap; - PrefObserverMap pref_observers_; - - friend class ScopedPrefUpdate; - DISALLOW_COPY_AND_ASSIGN(PrefService); }; diff --git a/chrome/browser/pref_value_store.cc b/chrome/browser/pref_value_store.cc index 47b01e0..d5c7ab4 100644 --- a/chrome/browser/pref_value_store.cc +++ b/chrome/browser/pref_value_store.cc @@ -46,7 +46,7 @@ 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. - for (size_t i = 0; i <= PREF_STORE_TYPE_MAX; ++i) { + 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)) { return true; @@ -59,7 +59,7 @@ bool PrefValueStore::GetValue(const std::string& name, bool PrefValueStore::WritePrefs() { bool success = true; - for (size_t i = 0; i <= PREF_STORE_TYPE_MAX; ++i) { + for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) { if (pref_stores_[i].get()) success = pref_stores_[i]->WritePrefs() && success; } @@ -67,7 +67,7 @@ bool PrefValueStore::WritePrefs() { } void PrefValueStore::ScheduleWritePrefs() { - for (size_t i = 0; i <= PREF_STORE_TYPE_MAX; ++i) { + for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) { if (pref_stores_[i].get()) pref_stores_[i]->ScheduleWritePrefs(); } @@ -75,7 +75,7 @@ void PrefValueStore::ScheduleWritePrefs() { PrefStore::PrefReadError PrefValueStore::ReadPrefs() { PrefStore::PrefReadError result = PrefStore::PREF_READ_ERROR_NONE; - for (size_t i = 0; i <= PREF_STORE_TYPE_MAX; ++i) { + for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) { if (pref_stores_[i].get()) { PrefStore::PrefReadError this_error = pref_stores_[i]->ReadPrefs(); if (result == PrefStore::PREF_READ_ERROR_NONE) @@ -94,49 +94,60 @@ bool PrefValueStore::HasPrefPath(const char* path) const { return rv; } +bool PrefValueStore::PrefHasChanged(const char* path, + const Value* old_value) { + Value* new_value = NULL; + GetValue(path, &new_value); + // Some unit tests have no values for certain prefs. + return (!new_value || !old_value->Equals(new_value)); +} + // Note the |DictionaryValue| referenced by the |PrefStore| user_prefs_ // (returned by the method prefs()) takes the ownership of the Value referenced // by in_value. void PrefValueStore::SetUserPrefValue(const char* name, Value* in_value) { - pref_stores_[USER]->prefs()->Set(name, in_value); + pref_stores_[PrefNotifier::USER_STORE]->prefs()->Set(name, in_value); } bool PrefValueStore::ReadOnly() { - return pref_stores_[USER]->ReadOnly(); + return pref_stores_[PrefNotifier::USER_STORE]->ReadOnly(); } void PrefValueStore::RemoveUserPrefValue(const char* name) { - if (pref_stores_[USER].get()) { - pref_stores_[USER]->prefs()->Remove(name, NULL); + if (pref_stores_[PrefNotifier::USER_STORE].get()) { + pref_stores_[PrefNotifier::USER_STORE]->prefs()->Remove(name, NULL); } } bool PrefValueStore::PrefValueInManagedStore(const char* name) { - return PrefValueInStore(name, MANAGED); + return PrefValueInStore(name, PrefNotifier::MANAGED_STORE); } bool PrefValueStore::PrefValueInExtensionStore(const char* name) { - return PrefValueInStore(name, EXTENSION); + return PrefValueInStore(name, PrefNotifier::EXTENSION_STORE); } bool PrefValueStore::PrefValueInUserStore(const char* name) { - return PrefValueInStore(name, USER); + return PrefValueInStore(name, PrefNotifier::USER_STORE); } bool PrefValueStore::PrefValueFromExtensionStore(const char* name) { - return ControllingPrefStoreForPref(name) == EXTENSION; + return ControllingPrefStoreForPref(name) == PrefNotifier::EXTENSION_STORE; } bool PrefValueStore::PrefValueFromUserStore(const char* name) { - return ControllingPrefStoreForPref(name) == USER; + return ControllingPrefStoreForPref(name) == PrefNotifier::USER_STORE; } bool PrefValueStore::PrefValueUserModifiable(const char* name) { - PrefStoreType effective_store = ControllingPrefStoreForPref(name); - return effective_store >= USER || effective_store == INVALID; + PrefNotifier::PrefStoreType effective_store = + ControllingPrefStoreForPref(name); + return effective_store >= PrefNotifier::USER_STORE || + effective_store == PrefNotifier::INVALID_STORE; } -bool PrefValueStore::PrefValueInStore(const char* name, PrefStoreType type) { +bool PrefValueStore::PrefValueInStore(const char* name, + PrefNotifier::PrefStoreType type) { if (!pref_stores_[type].get()) { // No store of that type set, so this pref can't be in it. return false; @@ -145,13 +156,13 @@ bool PrefValueStore::PrefValueInStore(const char* name, PrefStoreType type) { return pref_stores_[type]->prefs()->Get(name, &tmp_value); } -PrefValueStore::PrefStoreType PrefValueStore::ControllingPrefStoreForPref( +PrefNotifier::PrefStoreType PrefValueStore::ControllingPrefStoreForPref( const char* name) { - for (int i = 0; i <= PREF_STORE_TYPE_MAX; ++i) { - if (PrefValueInStore(name, static_cast<PrefStoreType>(i))) - return static_cast<PrefStoreType>(i); + for (int i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) { + if (PrefValueInStore(name, static_cast<PrefNotifier::PrefStoreType>(i))) + return static_cast<PrefNotifier::PrefStoreType>(i); } - return INVALID; + return PrefNotifier::INVALID_STORE; } void PrefValueStore::RefreshPolicyPrefsCompletion( @@ -159,9 +170,11 @@ void PrefValueStore::RefreshPolicyPrefsCompletion( PrefStore* new_recommended_pref_store, AfterRefreshCallback* callback_pointer) { scoped_ptr<AfterRefreshCallback> callback(callback_pointer); - DictionaryValue* managed_prefs_before(pref_stores_[MANAGED]->prefs()); + DictionaryValue* managed_prefs_before( + pref_stores_[PrefNotifier::MANAGED_STORE]->prefs()); DictionaryValue* managed_prefs_after(new_managed_pref_store->prefs()); - DictionaryValue* recommended_prefs_before(pref_stores_[RECOMMENDED]->prefs()); + DictionaryValue* recommended_prefs_before( + pref_stores_[PrefNotifier::RECOMMENDED_STORE]->prefs()); DictionaryValue* recommended_prefs_after(new_recommended_pref_store->prefs()); std::vector<std::string> changed_managed_paths; @@ -182,8 +195,9 @@ void PrefValueStore::RefreshPolicyPrefsCompletion( changed_paths.begin()); changed_paths.resize(last_insert - changed_paths.begin()); - pref_stores_[MANAGED].reset(new_managed_pref_store); - pref_stores_[RECOMMENDED].reset(new_recommended_pref_store); + pref_stores_[PrefNotifier::MANAGED_STORE].reset(new_managed_pref_store); + pref_stores_[PrefNotifier::RECOMMENDED_STORE].reset( + new_recommended_pref_store); callback->Run(changed_paths); } @@ -241,9 +255,9 @@ PrefValueStore::PrefValueStore(PrefStore* managed_prefs, PrefStore* command_line_prefs, PrefStore* user_prefs, PrefStore* recommended_prefs) { - pref_stores_[MANAGED].reset(managed_prefs); - pref_stores_[EXTENSION].reset(extension_prefs); - pref_stores_[COMMAND_LINE].reset(command_line_prefs); - pref_stores_[USER].reset(user_prefs); - pref_stores_[RECOMMENDED].reset(recommended_prefs); + pref_stores_[PrefNotifier::MANAGED_STORE].reset(managed_prefs); + pref_stores_[PrefNotifier::EXTENSION_STORE].reset(extension_prefs); + pref_stores_[PrefNotifier::COMMAND_LINE_STORE].reset(command_line_prefs); + pref_stores_[PrefNotifier::USER_STORE].reset(user_prefs); + pref_stores_[PrefNotifier::RECOMMENDED_STORE].reset(recommended_prefs); } diff --git a/chrome/browser/pref_value_store.h b/chrome/browser/pref_value_store.h index 9dfa56e..c1901f6 100644 --- a/chrome/browser/pref_value_store.h +++ b/chrome/browser/pref_value_store.h @@ -18,23 +18,19 @@ #include "base/scoped_ptr.h" #include "base/values.h" #include "chrome/browser/chrome_thread.h" +#include "chrome/browser/pref_notifier.h" #include "chrome/common/pref_store.h" class PrefStore; class Profile; -// The class PrefValueStore provides values for preferences. Each Preference -// has a unique name. This name is used to retrieve the value of a Preference. -// The value of a preference can be either managed, user-defined or recommended. -// Managed preference values are set (managed) by a third person (like an -// admin for example). They have the highest priority and can not be -// altered by the user. -// User-defined values are chosen by the user. If there is already -// a managed value for a preference the user-defined value is ignored and -// the managed value is used (returned). -// Otherwise user-defined values have a higher precedence than recommended -// values. Recommended preference values are set by a third person -// (like an admin). +// The PrefValueStore manages various sources of values for Preferences +// (e.g., configuration policies, extensions, and user settings). It returns +// the value of a Preference from the source with the highest priority, and +// allows setting user-defined values for preferences that are not managed. +// See PrefNotifier for a list of the available preference sources (PrefStores) +// and their descriptions. +// // Unless otherwise explicitly noted, all of the methods of this class must // be called on the UI thread. class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> { @@ -74,6 +70,11 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> { // Returns true if the PrefValueStore contains the given preference. bool HasPrefPath(const char* name) const; + // Returns true if the effective value of the preference has changed from its + // |old_value|. Virtual so it can be mocked for a unit test. + // TODO(pamg): Also return true when the controlling PrefStore changes. + virtual bool PrefHasChanged(const char* path, const Value* old_value); + // Returns true if the PrefValueStore is read-only. // Because the managed and recommended PrefStores are always read-only, the // PrefValueStore as a whole is read-only if the PrefStore containing the user @@ -151,27 +152,14 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> { FRIEND_TEST_ALL_PREFIXES(PrefValueStoreTest, TestRefreshPolicyPrefsCompletion); - // PrefStores must be listed here in order from highest to lowest priority. - enum PrefStoreType { - // Not associated with an actual PrefStore but used as invalid marker, e.g. - // as return value. - INVALID = -1, - MANAGED = 0, - EXTENSION, - COMMAND_LINE, - USER, - RECOMMENDED, - PREF_STORE_TYPE_MAX = RECOMMENDED - }; - - scoped_ptr<PrefStore> pref_stores_[PREF_STORE_TYPE_MAX + 1]; + scoped_ptr<PrefStore> pref_stores_[PrefNotifier::PREF_STORE_TYPE_MAX + 1]; - bool PrefValueInStore(const char* name, PrefStoreType type); + bool PrefValueInStore(const char* name, PrefNotifier::PrefStoreType type); // Returns the pref store type identifying the source that controls the // Preference identified by |name|. If none of the sources has a value, // INVALID is returned. - PrefStoreType ControllingPrefStoreForPref(const char* name); + PrefNotifier::PrefStoreType ControllingPrefStoreForPref(const char* name); // Called during policy refresh after ReadPrefs completes on the thread // that initiated the policy refresh. RefreshPolicyPrefsCompletion takes diff --git a/chrome/browser/pref_value_store_unittest.cc b/chrome/browser/pref_value_store_unittest.cc index 0effd04..c159945 100644 --- a/chrome/browser/pref_value_store_unittest.cc +++ b/chrome/browser/pref_value_store_unittest.cc @@ -15,6 +15,16 @@ using testing::_; using testing::Mock; +namespace { + +class MockPolicyRefreshCallback { + public: + MockPolicyRefreshCallback() {} + MOCK_METHOD1(DoCallback, void(const std::vector<std::string>)); +}; + +} // namespace + // Names of the preferences used in this test program. namespace prefs { const char kCurrentThemeID[] = "extensions.theme.id"; @@ -31,7 +41,7 @@ namespace prefs { const char kApplicationLocale[] = "intl.app_locale"; } -// Potentailly expected values of all preferences used in this test program. +// 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 { @@ -281,6 +291,25 @@ TEST_F(PrefValueStoreTest, HasPrefPath) { EXPECT_FALSE(pref_value_store_->HasPrefPath(prefs::kMissingPref)); } +TEST_F(PrefValueStoreTest, PrefHasChanged) { + ASSERT_TRUE(pref_value_store_->PrefValueInManagedStore(prefs::kHomepage)); + + // Pretend we used to have a different enforced value set. + scoped_ptr<Value> value(Value::CreateStringValue("http://www.youtube.com")); + EXPECT_TRUE(pref_value_store_->PrefHasChanged(prefs::kHomepage, value.get())); + + // Pretend we used to have the same enforced value set. + value.reset(Value::CreateStringValue(enforced_pref::kHomepageValue)); + EXPECT_FALSE(pref_value_store_->PrefHasChanged(prefs::kHomepage, + value.get())); + + // Really set a new value in a lower-priority store. + Value* new_value = Value::CreateStringValue("http://www.chromium.org"); + pref_value_store_->SetUserPrefValue(prefs::kHomepage, new_value); + EXPECT_FALSE(pref_value_store_->PrefHasChanged(prefs::kHomepage, + value.get())); +} + TEST_F(PrefValueStoreTest, ReadPrefs) { pref_value_store_->ReadPrefs(); // The ReadPrefs method of the |DummyPrefStore| deletes the |pref_store|s @@ -456,12 +485,6 @@ TEST_F(PrefValueStoreTest, PrefValueInUserStore) { EXPECT_FALSE(pref_value_store_->PrefValueFromUserStore(prefs::kMissingPref)); } -class MockPolicyRefreshCallback { - public: - MockPolicyRefreshCallback() {} - MOCK_METHOD1(DoCallback, void(const std::vector<std::string>)); -}; - TEST_F(PrefValueStoreTest, TestPolicyRefresh) { // pref_value_store_ is initialized by PrefValueStoreTest to have values // in both it's managed and recommended store. By replacing them with diff --git a/chrome/browser/scoped_pref_update.cc b/chrome/browser/scoped_pref_update.cc index 5e74ee3..bc8c382 100644 --- a/chrome/browser/scoped_pref_update.cc +++ b/chrome/browser/scoped_pref_update.cc @@ -16,5 +16,5 @@ ScopedPrefUpdate::ScopedPrefUpdate(PrefService* service, const wchar_t* path) path_(WideToUTF8(path)) {} ScopedPrefUpdate::~ScopedPrefUpdate() { - service_->FireObservers(path_.c_str()); + service_->pref_notifier()->FireObservers(path_.c_str()); } |