Add Closure-based API to PrefChangeObserver and PrefMember, deprecate PrefObserver-based API.

This switches the API with minimal implementation changes; PrefNotifierImpl still uses PrefObserver to accept registrations, and still filters on pref names (which PrefChangeObserver looks up again when it receives callbacks via its PrefObserver implementation).

This approach is chosen for now to establish the new API so that usages can be switched, because:

a) We need a way for PrefNotifierImpl to dispatch to both PrefChangeObserver and PrefMember, and it's unclear to me whether we want to switch that interface to base::Callback as well (if so, how to unregister?), or whether we want PrefMember to have a PrefChangeObserver instance (probably inefficient?) or something else.

b) There are plans to do performance measurements of a few different implementation approaches in how PrefNotifierImpl and PrefChangeObserver interact; that interaction can be changed "under the hood" while the new API stays unchanged, and this lets us start switching users to the new API and removing the now-deprecated PrefObserver-based API.

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

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

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

3 files changed, 70 insertions, 57 deletions

diff --git a/base/prefs/public/pref_change_registrar.cc b/base/prefs/public/pref_change_registrar.cc
index 3f94b04..47efc9c 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,37 @@ 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_, std::string(path)));
 }
 
-void PrefChangeRegistrar::Remove(const char* path, PrefObserver* obs) {
+void PrefChangeRegistrar::Add(const char* path, const base::Closure& 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 +62,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 +75,9 @@ bool PrefChangeRegistrar::IsManaged() {
   }
   return false;
 }
+
+void PrefChangeRegistrar::OnPreferenceChanged(PrefServiceBase* service,
+                                              const std::string& pref) {
+  if (IsObserved(pref))
+    observers_[pref].Run();
+}
diff --git a/base/prefs/public/pref_change_registrar.h b/base/prefs/public/pref_change_registrar.h
index f3abd98..adb92ee 100644
--- a/base/prefs/public/pref_change_registrar.h
+++ b/base/prefs/public/pref_change_registrar.h
@@ -5,20 +5,21 @@
 #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:
   PrefChangeRegistrar();
   virtual ~PrefChangeRegistrar();
@@ -29,13 +30,16 @@ class BASE_PREFS_EXPORT PrefChangeRegistrar {
 
   // Adds an 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);
+
+  // 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 +54,13 @@ 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;
+
+  typedef std::map<std::string, base::Closure> 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_;
 };