Reland: Closure-based API to PrefChangeObserver and PrefMember.

The original was in http://codereview.chromium.org/11368098/ (landed
as r166670) but seemed to increase memory usage significantly.

This version does away with storing the pref name in callbacks, which
should reduce memory usage.

TBR=ben@chromium.org,finnur@chromium.org
BUG=155525, 160177

Review URL: https://chromiumcodereview.appspot.com/11369153

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

3 files changed, 89 insertions, 58 deletions

diff --git a/base/prefs/public/pref_change_registrar.cc b/base/prefs/public/pref_change_registrar.cc
index 3f94b04..0a442e7 100644
--- a/base/prefs/public/pref_change_registrar.cc
+++ b/base/prefs/public/pref_change_registrar.cc
@@ -4,6 +4,7 @@
 
 #include "base/prefs/public/pref_change_registrar.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/prefs/public/pref_service_base.h"
 
@@ -23,43 +24,42 @@ void PrefChangeRegistrar::Init(PrefServiceBase* service) {
 }
 
 void PrefChangeRegistrar::Add(const char* path, PrefObserver* obs) {
-  if (!service_) {
-    NOTREACHED();
-    return;
-  }
-  ObserverRegistration registration(path, obs);
-  if (observers_.find(registration) != observers_.end()) {
-    NOTREACHED();
-    return;
-  }
-  observers_.insert(registration);
-  service_->AddPrefObserver(path, obs);
+  DCHECK(obs);
+  return Add(path, base::Bind(&PrefObserver::OnPreferenceChanged,
+                              base::Unretained(obs), service_));
 }
 
-void PrefChangeRegistrar::Remove(const char* path, PrefObserver* obs) {
+void PrefChangeRegistrar::Add(const char* path,
+                              const base::Closure& obs) {
+  Add(path, base::Bind(&PrefChangeRegistrar::InvokeUnnamedCallback, obs));
+}
+
+void PrefChangeRegistrar::Add(const char* path,
+                              const NamedChangeCallback& obs) {
   if (!service_) {
     NOTREACHED();
     return;
   }
-  ObserverRegistration registration(path, obs);
-  std::set<ObserverRegistration>::iterator it =
-       observers_.find(registration);
-  if (it == observers_.end()) {
-    NOTREACHED();
-    return;
-  }
-  service_->RemovePrefObserver(it->first.c_str(), it->second);
-  observers_.erase(it);
+  DCHECK(!IsObserved(path)) << "Already had this pref registered.";
+
+  service_->AddPrefObserver(path, this);
+  observers_[path] = obs;
+}
+
+void PrefChangeRegistrar::Remove(const char* path) {
+  DCHECK(IsObserved(path));
+
+  observers_.erase(path);
+  service_->RemovePrefObserver(path, this);
 }
 
 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();
+  for (ObserverMap::const_iterator it = observers_.begin();
+       it != observers_.end(); ++it) {
+    service_->RemovePrefObserver(it->first.c_str(), this);
   }
+
+  observers_.clear();
 }
 
 bool PrefChangeRegistrar::IsEmpty() const {
@@ -67,16 +67,11 @@ bool PrefChangeRegistrar::IsEmpty() const {
 }
 
 bool PrefChangeRegistrar::IsObserved(const std::string& pref) {
-  for (std::set<ObserverRegistration>::const_iterator it = observers_.begin();
-       it != observers_.end(); ++it) {
-    if (it->first == pref)
-      return true;
-  }
-  return false;
+  return observers_.find(pref) != observers_.end();
 }
 
 bool PrefChangeRegistrar::IsManaged() {
-  for (std::set<ObserverRegistration>::const_iterator it = observers_.begin();
+  for (ObserverMap::const_iterator it = observers_.begin();
        it != observers_.end(); ++it) {
     const PrefServiceBase::Preference* pref =
         service_->FindPreference(it->first.c_str());
@@ -85,3 +80,14 @@ bool PrefChangeRegistrar::IsManaged() {
   }
   return false;
 }
+
+void PrefChangeRegistrar::OnPreferenceChanged(PrefServiceBase* service,
+                                              const std::string& pref) {
+  if (IsObserved(pref))
+    observers_[pref].Run(pref);
+}
+
+void PrefChangeRegistrar::InvokeUnnamedCallback(const base::Closure& callback,
+                                                const std::string& pref_name) {
+  callback.Run();
+}
diff --git a/base/prefs/public/pref_change_registrar.h b/base/prefs/public/pref_change_registrar.h
index f3abd98..a21cce0 100644
--- a/base/prefs/public/pref_change_registrar.h
+++ b/base/prefs/public/pref_change_registrar.h
@@ -5,21 +5,26 @@
 #ifndef BASE_PREFS_PUBLIC_PREF_CHANGE_REGISTRAR_H_
 #define BASE_PREFS_PUBLIC_PREF_CHANGE_REGISTRAR_H_
 
-#include <set>
+#include <map>
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/prefs/base_prefs_export.h"
+#include "base/prefs/public/pref_observer.h"
 
-class PrefObserver;
 class PrefServiceBase;
 
 // Automatically manages the registration of one or more pref change observers
 // with a PrefStore. Functions much like NotificationRegistrar, but specifically
 // manages observers of preference changes. When the Registrar is destroyed,
 // all registered observers are automatically unregistered with the PrefStore.
-class BASE_PREFS_EXPORT PrefChangeRegistrar {
+class BASE_PREFS_EXPORT PrefChangeRegistrar : public PrefObserver {
  public:
+  // You can register this type of callback if you need to know the
+  // path of the preference that is changing.
+  typedef base::Callback<void(const std::string&)> NamedChangeCallback;
+
   PrefChangeRegistrar();
   virtual ~PrefChangeRegistrar();
 
@@ -27,15 +32,19 @@ class BASE_PREFS_EXPORT PrefChangeRegistrar {
   // than once as long as the value of |service| doesn't change.
   void Init(PrefServiceBase* service);
 
-  // Adds an pref observer for the specified pref |path| and |obs| observer
+  // Adds a pref observer for the specified pref |path| and |obs| observer
   // object. All registered observers will be automatically unregistered
-  // when the registrar's destructor is called unless the observer has been
-  // explicitly removed by a call to Remove beforehand.
+  // when the registrar's destructor is called.
+  //
+  // Only one observer may be registered per path.
+  void Add(const char* path, const base::Closure& obs);
+  void Add(const char* path, const NamedChangeCallback& obs);
+
+  // Deprecated version of Add, soon to be removed.
   void Add(const char* path, PrefObserver* obs);
 
-  // Removes a preference observer that has previously been added with a call to
-  // Add.
-  void Remove(const char* path, PrefObserver* obs);
+  // Removes the pref observer registered for |path|.
+  void Remove(const char* path);
 
   // Removes all observers that have been previously added with a call to Add.
   void RemoveAll();
@@ -50,9 +59,16 @@ class BASE_PREFS_EXPORT PrefChangeRegistrar {
   bool IsManaged();
 
  private:
-  typedef std::pair<std::string, PrefObserver*> ObserverRegistration;
+  // PrefObserver:
+  virtual void OnPreferenceChanged(PrefServiceBase* service,
+                                   const std::string& pref_name) OVERRIDE;
+
+  static void InvokeUnnamedCallback(const base::Closure& callback,
+                                    const std::string& pref_name);
+
+  typedef std::map<std::string, NamedChangeCallback> ObserverMap;
 
-  std::set<ObserverRegistration> observers_;
+  ObserverMap observers_;
   PrefServiceBase* service_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefChangeRegistrar);
diff --git a/base/prefs/public/pref_change_registrar_unittest.cc b/base/prefs/public/pref_change_registrar_unittest.cc
index f860602..301a0ab 100644
--- a/base/prefs/public/pref_change_registrar_unittest.cc
+++ b/base/prefs/public/pref_change_registrar_unittest.cc
@@ -68,9 +68,9 @@ TEST_F(PrefChangeRegistrarTest, AddAndRemove) {
 
   // Test adding.
   EXPECT_CALL(*service(),
-              AddPrefObserver(Eq(std::string("test.pref.1")), observer()));
+              AddPrefObserver(Eq(std::string("test.pref.1")), &registrar));
   EXPECT_CALL(*service(),
-              AddPrefObserver(Eq(std::string("test.pref.2")), observer()));
+              AddPrefObserver(Eq(std::string("test.pref.2")), &registrar));
   registrar.Add("test.pref.1", observer());
   registrar.Add("test.pref.2", observer());
   EXPECT_FALSE(registrar.IsEmpty());
@@ -78,11 +78,11 @@ TEST_F(PrefChangeRegistrarTest, AddAndRemove) {
   // Test removing.
   Mock::VerifyAndClearExpectations(service());
   EXPECT_CALL(*service(),
-              RemovePrefObserver(Eq(std::string("test.pref.1")), observer()));
+              RemovePrefObserver(Eq(std::string("test.pref.1")), &registrar));
   EXPECT_CALL(*service(),
-              RemovePrefObserver(Eq(std::string("test.pref.2")), observer()));
-  registrar.Remove("test.pref.1", observer());
-  registrar.Remove("test.pref.2", observer());
+              RemovePrefObserver(Eq(std::string("test.pref.2")), &registrar));
+  registrar.Remove("test.pref.1");
+  registrar.Remove("test.pref.2");
   EXPECT_TRUE(registrar.IsEmpty());
 
   // Explicitly check the expectations now to make sure that the Removes
@@ -96,14 +96,14 @@ TEST_F(PrefChangeRegistrarTest, AutoRemove) {
 
   // Setup of auto-remove.
   EXPECT_CALL(*service(),
-              AddPrefObserver(Eq(std::string("test.pref.1")), observer()));
+              AddPrefObserver(Eq(std::string("test.pref.1")), &registrar));
   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()));
+              RemovePrefObserver(Eq(std::string("test.pref.1")), &registrar));
 }
 
 TEST_F(PrefChangeRegistrarTest, RemoveAll) {
@@ -111,17 +111,17 @@ TEST_F(PrefChangeRegistrarTest, RemoveAll) {
   registrar.Init(service());
 
   EXPECT_CALL(*service(),
-              AddPrefObserver(Eq(std::string("test.pref.1")), observer()));
+              AddPrefObserver(Eq(std::string("test.pref.1")), &registrar));
   EXPECT_CALL(*service(),
-              AddPrefObserver(Eq(std::string("test.pref.2")), observer()));
+              AddPrefObserver(Eq(std::string("test.pref.2")), &registrar));
   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()));
+              RemovePrefObserver(Eq(std::string("test.pref.1")), &registrar));
   EXPECT_CALL(*service(),
-              RemovePrefObserver(Eq(std::string("test.pref.2")), observer()));
+              RemovePrefObserver(Eq(std::string("test.pref.2")), &registrar));
   registrar.RemoveAll();
   EXPECT_TRUE(registrar.IsEmpty());
 
@@ -130,7 +130,8 @@ TEST_F(PrefChangeRegistrarTest, RemoveAll) {
   Mock::VerifyAndClearExpectations(service());
 }
 
-class ObserveSetOfPreferencesTest : public testing::Test {
+class ObserveSetOfPreferencesTest : public testing::Test,
+                                    public PrefObserver {
  public:
   virtual void SetUp() {
     pref_service_.reset(new TestingPrefService);
@@ -147,6 +148,8 @@ class ObserveSetOfPreferencesTest : public testing::Test {
 
   PrefChangeRegistrar* CreatePrefChangeRegistrar(
       PrefObserver* observer) {
+    if (!observer)
+      observer = this;
     PrefChangeRegistrar* pref_set = new PrefChangeRegistrar();
     pref_set->Init(pref_service_.get());
     pref_set->Add(prefs::kHomePage, observer);
@@ -154,6 +157,12 @@ class ObserveSetOfPreferencesTest : public testing::Test {
     return pref_set;
   }
 
+  // PrefObserver (used to enable NULL as parameter to
+  // CreatePrefChangeRegistrar above).
+  virtual void OnPreferenceChanged(PrefServiceBase* service,
+                                   const std::string& pref_name) OVERRIDE {
+  }
+
   scoped_ptr<TestingPrefService> pref_service_;
 };