From 2fb7dc983456e980d631501f4a120eb091d197e7 Mon Sep 17 00:00:00 2001 From: "danno@chromium.org" Date: Wed, 29 Sep 2010 12:24:28 +0000 Subject: Use PrefChangeRegistrar everywhere BUG=54955 TEST=PrefChangeRegistrarTest.* Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60169 Review URL: http://codereview.chromium.org/3304015 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60935 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/prefs/pref_change_registrar.cc | 23 +++++++++---- chrome/browser/prefs/pref_change_registrar.h | 9 ++++- .../prefs/pref_change_registrar_unittest.cc | 31 +++++++++++++++++ chrome/browser/prefs/pref_service.h | 26 +++++++++++--- chrome/browser/prefs/pref_service_unittest.cc | 40 ++++++++++++---------- chrome/browser/prefs/pref_set_observer.cc | 10 ++---- chrome/browser/prefs/pref_set_observer.h | 4 ++- 7 files changed, 103 insertions(+), 40 deletions(-) (limited to 'chrome/browser/prefs') diff --git a/chrome/browser/prefs/pref_change_registrar.cc b/chrome/browser/prefs/pref_change_registrar.cc index dc8bb97..05372b1 100644 --- a/chrome/browser/prefs/pref_change_registrar.cc +++ b/chrome/browser/prefs/pref_change_registrar.cc @@ -9,16 +9,11 @@ PrefChangeRegistrar::PrefChangeRegistrar() : service_(NULL) {} PrefChangeRegistrar::~PrefChangeRegistrar() { - if (service_) { - for (std::set::const_iterator it = observers_.begin(); - it != observers_.end(); ++it) { - service_->RemovePrefObserver(it->first.c_str(), it->second); - } - } + RemoveAll(); } void PrefChangeRegistrar::Init(PrefService* service) { - DCHECK(!service_); + DCHECK(IsEmpty() || service_ == service); service_ = service; } @@ -51,3 +46,17 @@ void PrefChangeRegistrar::Remove(const char* path, NotificationObserver* obs) { service_->RemovePrefObserver(it->first.c_str(), it->second); observers_.erase(it); } + +void PrefChangeRegistrar::RemoveAll() { + if (service_) { + for (std::set::const_iterator it = observers_.begin(); + it != observers_.end(); ++it) { + service_->RemovePrefObserver(it->first.c_str(), it->second); + } + observers_.clear(); + } +} + +bool PrefChangeRegistrar::IsEmpty() const { + return observers_.empty(); +} diff --git a/chrome/browser/prefs/pref_change_registrar.h b/chrome/browser/prefs/pref_change_registrar.h index cd8f5eb..773c556 100644 --- a/chrome/browser/prefs/pref_change_registrar.h +++ b/chrome/browser/prefs/pref_change_registrar.h @@ -23,7 +23,8 @@ class PrefChangeRegistrar { PrefChangeRegistrar(); virtual ~PrefChangeRegistrar(); - // Must be called before adding or removing observers. + // Must be called before adding or removing observers. Can be called more + // than once as long as the value of |service| doesn't change. void Init(PrefService* service); // Adds an pref observer for the specified pref |path| and |obs| observer @@ -38,6 +39,12 @@ class PrefChangeRegistrar { void Remove(const char* path, NotificationObserver* obs); + // Removes all observers that have been previously added with a call to Add. + void RemoveAll(); + + // Returns true if no pref observers are registered. + bool IsEmpty() const; + private: typedef std::pair ObserverRegistration; diff --git a/chrome/browser/prefs/pref_change_registrar_unittest.cc b/chrome/browser/prefs/pref_change_registrar_unittest.cc index 4d7f747..8096ee6 100644 --- a/chrome/browser/prefs/pref_change_registrar_unittest.cc +++ b/chrome/browser/prefs/pref_change_registrar_unittest.cc @@ -63,6 +63,7 @@ TEST_F(PrefChangeRegistrarTest, AddAndRemove) { AddPrefObserver(Eq(std::string("test.pref.2")), observer())); registrar.Add("test.pref.1", observer()); registrar.Add("test.pref.2", observer()); + EXPECT_FALSE(registrar.IsEmpty()); // Test removing. Mock::VerifyAndClearExpectations(service()); @@ -72,6 +73,11 @@ TEST_F(PrefChangeRegistrarTest, AddAndRemove) { RemovePrefObserver(Eq(std::string("test.pref.2")), observer())); registrar.Remove("test.pref.1", observer()); registrar.Remove("test.pref.2", observer()); + EXPECT_TRUE(registrar.IsEmpty()); + + // Explicitly check the expectations now to make sure that the Removes + // worked (rather than the registrar destructor doing the work). + Mock::VerifyAndClearExpectations(service()); } TEST_F(PrefChangeRegistrarTest, AutoRemove) { @@ -83,8 +89,33 @@ TEST_F(PrefChangeRegistrarTest, AutoRemove) { AddPrefObserver(Eq(std::string("test.pref.1")), observer())); registrar.Add("test.pref.1", observer()); Mock::VerifyAndClearExpectations(service()); + EXPECT_FALSE(registrar.IsEmpty()); // Test auto-removing. EXPECT_CALL(*service(), RemovePrefObserver(Eq(std::string("test.pref.1")), observer())); } + +TEST_F(PrefChangeRegistrarTest, RemoveAll) { + PrefChangeRegistrar registrar; + registrar.Init(service()); + + EXPECT_CALL(*service(), + AddPrefObserver(Eq(std::string("test.pref.1")), observer())); + EXPECT_CALL(*service(), + AddPrefObserver(Eq(std::string("test.pref.2")), observer())); + registrar.Add("test.pref.1", observer()); + registrar.Add("test.pref.2", observer()); + Mock::VerifyAndClearExpectations(service()); + + EXPECT_CALL(*service(), + RemovePrefObserver(Eq(std::string("test.pref.1")), observer())); + EXPECT_CALL(*service(), + RemovePrefObserver(Eq(std::string("test.pref.2")), observer())); + registrar.RemoveAll(); + EXPECT_TRUE(registrar.IsEmpty()); + + // Explicitly check the expectations now to make sure that the RemoveAll + // worked (rather than the registrar destructor doing the work). + Mock::VerifyAndClearExpectations(service()); +} diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h index 450c6c1..5cedd52 100644 --- a/chrome/browser/prefs/pref_service.h +++ b/chrome/browser/prefs/pref_service.h @@ -19,9 +19,14 @@ class FilePath; class NotificationObserver; +class PrefChangeObserver; class PrefNotifier; class Profile; +namespace subtle { + class PrefMemberBase; +}; + class PrefService : public NonThreadSafe { public: // A helper class to store all the information associated with a preference. @@ -161,11 +166,6 @@ class PrefService : public NonThreadSafe { const DictionaryValue* GetDictionary(const char* path) const; const ListValue* GetList(const char* path) const; - // If the pref at the given path changes, we call the observer's Observe - // method with NOTIFY_PREF_CHANGED. - virtual void AddPrefObserver(const char* path, NotificationObserver* obs); - virtual void RemovePrefObserver(const char* path, NotificationObserver* obs); - // Removes a user pref and restores the pref to its default value. void ClearPref(const char* path); @@ -229,6 +229,22 @@ class PrefService : public NonThreadSafe { scoped_ptr pref_notifier_; private: + // Registration of pref change observers must be done using the + // PrefChangeRegistrar, which is declared as a friend here to grant it + // access to the otherwise protected members Add/RemovePrefObserver. + // PrefMember registers for preferences changes notification directly to + // avoid the storage overhead of the registrar, so its base class must be + // declared as a friend, too. + friend class PrefChangeRegistrar; + friend class subtle::PrefMemberBase; + + // If the pref at the given path changes, we call the observer's Observe + // method with NOTIFY_PREF_CHANGED. Note that observers should not call + // these methods directly but rather use a PrefChangeRegistrar to make sure + // the observer gets cleaned up properly. + virtual void AddPrefObserver(const char* path, NotificationObserver* obs); + virtual void RemovePrefObserver(const char* path, NotificationObserver* obs); + // Add a preference to the PreferenceMap. If the pref already exists, return // false. This method takes ownership of |default_value|. void RegisterPreference(const char* path, Value* default_value); diff --git a/chrome/browser/prefs/pref_service_unittest.cc b/chrome/browser/prefs/pref_service_unittest.cc index 461f108..d62679d 100644 --- a/chrome/browser/prefs/pref_service_unittest.cc +++ b/chrome/browser/prefs/pref_service_unittest.cc @@ -8,6 +8,7 @@ #include "base/scoped_ptr.h" #include "base/values.h" #include "chrome/browser/prefs/dummy_pref_store.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_value_store.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/notification_observer_mock.h" @@ -94,7 +95,10 @@ TEST(PrefServiceTest, NoObserverFire) { const std::string new_pref_value("http://www.google.com/"); TestPrefObserver obs(&prefs, pref_name, new_pref_value); - prefs.AddPrefObserver(pref_name, &obs); + + PrefChangeRegistrar registrar; + registrar.Init(&prefs); + registrar.Add(pref_name, &obs); // This should fire the checks in TestPrefObserver::Observe. prefs.SetString(pref_name, new_pref_value); @@ -116,9 +120,6 @@ TEST(PrefServiceTest, NoObserverFire) { obs.Reset(""); prefs.ClearPref(pref_name); EXPECT_FALSE(obs.observer_fired()); - - // Ok, clean up. - prefs.RemovePrefObserver(pref_name, &obs); } TEST(PrefServiceTest, HasPrefPath) { @@ -148,7 +149,9 @@ TEST(PrefServiceTest, Observers) { const std::string new_pref_value("http://www.google.com/"); TestPrefObserver obs(&prefs, pref_name, new_pref_value); - prefs.AddPrefObserver(pref_name, &obs); + PrefChangeRegistrar registrar; + registrar.Init(&prefs); + registrar.Add(pref_name, &obs); // This should fire the checks in TestPrefObserver::Observe. prefs.SetString(pref_name, new_pref_value); @@ -159,23 +162,20 @@ TEST(PrefServiceTest, Observers) { const std::string new_pref_value2("http://www.youtube.com/"); obs.Reset(new_pref_value2); TestPrefObserver obs2(&prefs, pref_name, new_pref_value2); - prefs.AddPrefObserver(pref_name, &obs2); + registrar.Add(pref_name, &obs2); // This should fire the checks in obs and obs2. prefs.SetString(pref_name, new_pref_value2); EXPECT_TRUE(obs.observer_fired()); EXPECT_TRUE(obs2.observer_fired()); // Make sure obs2 still works after removing obs. - prefs.RemovePrefObserver(pref_name, &obs); + registrar.Remove(pref_name, &obs); obs.Reset(""); obs2.Reset(new_pref_value); // This should only fire the observer in obs2. prefs.SetString(pref_name, new_pref_value); EXPECT_FALSE(obs.observer_fired()); EXPECT_TRUE(obs2.observer_fired()); - - // Ok, clean up. - prefs.RemovePrefObserver(pref_name, &obs2); } class PrefServiceSetValueTest : public testing::Test { @@ -211,7 +211,11 @@ TEST_F(PrefServiceSetValueTest, SetStringValue) { const char default_string[] = "default"; scoped_ptr default_value(Value::CreateStringValue(default_string)); prefs_.RegisterStringPref(name_, default_string); - prefs_.AddPrefObserver(name_, &observer_); + + PrefChangeRegistrar registrar; + registrar.Init(&prefs_); + registrar.Add(name_, &observer_); + // Changing the controlling store from default to user triggers notification. SetExpectPrefChanged(); prefs_.Set(name_, *default_value); @@ -225,13 +229,13 @@ TEST_F(PrefServiceSetValueTest, SetStringValue) { SetExpectPrefChanged(); prefs_.Set(name_, *new_value); EXPECT_EQ(value_, prefs_.GetString(name_)); - - prefs_.RemovePrefObserver(name_, &observer_); } TEST_F(PrefServiceSetValueTest, SetDictionaryValue) { prefs_.RegisterDictionaryPref(name_); - prefs_.AddPrefObserver(name_, &observer_); + PrefChangeRegistrar registrar; + registrar.Init(&prefs_); + registrar.Add(name_, &observer_); // Dictionary values are special: setting one to NULL is the same as clearing // the user value, allowing the NULL default to take (or keep) control. @@ -259,13 +263,13 @@ TEST_F(PrefServiceSetValueTest, SetDictionaryValue) { Mock::VerifyAndClearExpectations(&observer_); dict = prefs_.GetMutableDictionary(name_); EXPECT_EQ(0U, dict->size()); - - prefs_.RemovePrefObserver(name_, &observer_); } TEST_F(PrefServiceSetValueTest, SetListValue) { prefs_.RegisterListPref(name_); - prefs_.AddPrefObserver(name_, &observer_); + PrefChangeRegistrar registrar; + registrar.Init(&prefs_); + registrar.Add(name_, &observer_); // List values are special: setting one to NULL is the same as clearing the // user value, allowing the NULL default to take (or keep) control. @@ -293,6 +297,4 @@ TEST_F(PrefServiceSetValueTest, SetListValue) { Mock::VerifyAndClearExpectations(&observer_); list = prefs_.GetMutableList(name_); EXPECT_EQ(0U, list->GetSize()); - - prefs_.RemovePrefObserver(name_, &observer_); } diff --git a/chrome/browser/prefs/pref_set_observer.cc b/chrome/browser/prefs/pref_set_observer.cc index a4ecf6f..dd067d3 100644 --- a/chrome/browser/prefs/pref_set_observer.cc +++ b/chrome/browser/prefs/pref_set_observer.cc @@ -11,23 +11,19 @@ PrefSetObserver::PrefSetObserver(PrefService* pref_service, NotificationObserver* observer) : pref_service_(pref_service), observer_(observer) { -} - -PrefSetObserver::~PrefSetObserver() { - for (PrefSet::const_iterator i(prefs_.begin()); i != prefs_.end(); ++i) - pref_service_->RemovePrefObserver(i->c_str(), this); + registrar_.Init(pref_service); } void PrefSetObserver::AddPref(const std::string& pref) { if (!prefs_.count(pref) && pref_service_->FindPreference(pref.c_str())) { prefs_.insert(pref); - pref_service_->AddPrefObserver(pref.c_str(), this); + registrar_.Add(pref.c_str(), this); } } void PrefSetObserver::RemovePref(const std::string& pref) { if (prefs_.erase(pref)) - pref_service_->RemovePrefObserver(pref.c_str(), this); + registrar_.Remove(pref.c_str(), this); } bool PrefSetObserver::IsObserved(const std::string& pref) { diff --git a/chrome/browser/prefs/pref_set_observer.h b/chrome/browser/prefs/pref_set_observer.h index beaec01..ff350df 100644 --- a/chrome/browser/prefs/pref_set_observer.h +++ b/chrome/browser/prefs/pref_set_observer.h @@ -10,6 +10,7 @@ #include "base/basictypes.h" #include "chrome/browser/prefs/pref_service.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/notification_observer.h" // Observes the state of a set of preferences and allows to query their combined @@ -19,7 +20,7 @@ class PrefSetObserver : public NotificationObserver { // Initialize with an empty set of preferences. PrefSetObserver(PrefService* pref_service, NotificationObserver* observer); - virtual ~PrefSetObserver(); + virtual ~PrefSetObserver() {} // Add a |pref| to the set of preferences to observe. void AddPref(const std::string& pref); @@ -51,6 +52,7 @@ class PrefSetObserver : public NotificationObserver { PrefSet prefs_; PrefService* pref_service_; + PrefChangeRegistrar registrar_; NotificationObserver* observer_; DISALLOW_COPY_AND_ASSIGN(PrefSetObserver); -- cgit v1.1