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

7 files changed, 103 insertions, 40 deletions

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<ObserverRegistration>::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<ObserverRegistration>::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<std::string, NotificationObserver*> 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<PrefNotifier> 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<Value> 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);