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

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();
+}

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;
 
-  std::set<ObserverRegistration> observers_;
+  typedef std::map<std::string, base::Closure> ObserverMap;
+
+  ObserverMap observers_;
   PrefServiceBase* service_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefChangeRegistrar);

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_;
 };
 

chrome/browser/api/prefs/pref_member.cc

@@ -4,7 +4,8 @@
 
 #include "chrome/browser/api/prefs/pref_member.h"
 
-#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/location.h"
 #include "base/prefs/public/pref_service_base.h"
 #include "base/value_conversions.h"
@@ -14,7 +15,7 @@ using base::MessageLoopProxy;
 namespace subtle {
 
 PrefMemberBase::PrefMemberBase()
-    : observer_(NULL),
+    : observer_(base::Bind(&base::DoNothing)),
       prefs_(NULL),
       setting_value_(false) {
 }
@@ -25,11 +26,16 @@ PrefMemberBase::~PrefMemberBase() {
 
 void PrefMemberBase::Init(const char* pref_name,
                           PrefServiceBase* prefs,
-                          PrefObserver* observer) {
+                          const base::Closure& observer) {
+  observer_ = observer;
+  Init(pref_name, prefs);
+}
+
+void PrefMemberBase::Init(const char* pref_name,
+                          PrefServiceBase* prefs) {
   DCHECK(pref_name);
   DCHECK(prefs);
   DCHECK(pref_name_.empty());  // Check that Init is only called once.
-  observer_ = observer;
   prefs_ = prefs;
   pref_name_ = pref_name;
   // Check that the preference is registered.
@@ -60,8 +66,8 @@ void PrefMemberBase::OnPreferenceChanged(PrefServiceBase* service,
                                          const std::string& pref_name) {
   VerifyValuePrefName();
   UpdateValueFromPref();
-  if (!setting_value_ && observer_)
-    observer_->OnPreferenceChanged(service, pref_name);
+  if (!setting_value_)
+    observer_.Run();
 }
 
 void PrefMemberBase::UpdateValueFromPref() const {

chrome/browser/api/prefs/pref_member.h

@@ -28,6 +28,8 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback_forward.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ -91,7 +93,8 @@ class PrefMemberBase : public PrefObserver {
 
   // See PrefMember<> for description.
   void Init(const char* pref_name, PrefServiceBase* prefs,
-            PrefObserver* observer);
+            const base::Closure& observer);
+  void Init(const char* pref_name, PrefServiceBase* prefs);
 
   virtual void CreateInternal() const = 0;
 
@@ -126,7 +129,7 @@ class PrefMemberBase : public PrefObserver {
  private:
   // Ordered the members to compact the class instance.
   std::string pref_name_;
-  PrefObserver* observer_;
+  base::Closure observer_;  // Initially bound to base::DoNothing.
   PrefServiceBase* prefs_;
 
  protected:
@@ -148,12 +151,28 @@ class PrefMember : public subtle::PrefMemberBase {
   PrefMember() {}
   virtual ~PrefMember() {}
 
-  // Do the actual initialization of the class.  |observer| may be null if you
-  // don't want any notifications of changes.
-  // This method should only be called on the UI thread.
+  // Do the actual initialization of the class.  Use the two-parameter
+  // version if you don't want any notifications of changes.  This
+  // method should only be called on the UI thread.
+  void Init(const char* pref_name, PrefServiceBase* prefs,
+            const base::Closure& observer) {
+    subtle::PrefMemberBase::Init(pref_name, prefs, observer);
+  }
+
+  void Init(const char* pref_name, PrefServiceBase* prefs) {
+    subtle::PrefMemberBase::Init(pref_name, prefs);
+  }
+
+  // Deprecated version of Init.
   void Init(const char* pref_name, PrefServiceBase* prefs,
             PrefObserver* observer) {
-    subtle::PrefMemberBase::Init(pref_name, prefs, observer);
+    if (observer) {
+      Init(pref_name, prefs, base::Bind(&PrefObserver::OnPreferenceChanged,
+                                        base::Unretained(observer),
+                                        prefs, std::string(pref_name)));
+    } else {
+      Init(pref_name, prefs);
+    }
   }
 
   // Unsubscribes the PrefMember from the PrefService. After calling this

chrome/browser/extensions/extension_prefs_unittest.cc

@@ -1129,8 +1129,8 @@ class ExtensionPrefsNotifyWhenNeeded : public ExtensionPrefsPrepopulatedTest {
     Mock::VerifyAndClearExpectations(&observer);
     Mock::VerifyAndClearExpectations(&incognito_observer);
 
-    registrar.Remove(kPref1, &observer);
-    incognito_registrar.Remove(kPref1, &incognito_observer);
+    registrar.Remove(kPref1);
+    incognito_registrar.Remove(kPref1);
   }
   virtual void Verify() {
     std::string actual = prefs()->pref_service()->GetString(kPref1);

chrome/browser/prefs/pref_service_unittest.cc

@@ -108,6 +108,9 @@ TEST(PrefServiceTest, Observers) {
   registrar.Init(&prefs);
   registrar.Add(pref_name, &obs);
 
+  PrefChangeRegistrar registrar_two;
+  registrar_two.Init(&prefs);
+
   // This should fire the checks in PrefObserverMock::Observe.
   obs.Expect(&prefs, pref_name, &expected_new_pref_value);
   prefs.SetString(pref_name, new_pref_value);
@@ -119,7 +122,7 @@ TEST(PrefServiceTest, Observers) {
   PrefObserverMock obs2;
   obs.Expect(&prefs, pref_name, &expected_new_pref_value2);
   obs2.Expect(&prefs, pref_name, &expected_new_pref_value2);
-  registrar.Add(pref_name, &obs2);
+  registrar_two.Add(pref_name, &obs2);
   // This should fire the checks in obs and obs2.
   prefs.SetString(pref_name, new_pref_value2);
   Mock::VerifyAndClearExpectations(&obs);
@@ -136,7 +139,7 @@ TEST(PrefServiceTest, Observers) {
   Mock::VerifyAndClearExpectations(&obs2);
 
   // Make sure obs2 still works after removing obs.
-  registrar.Remove(pref_name, &obs);
+  registrar.Remove(pref_name);
   EXPECT_CALL(obs, OnPreferenceChanged(_, _)).Times(0);
   obs2.Expect(&prefs, pref_name, &expected_new_pref_value);
   // This should only fire the observer in obs2.

chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

@@ -842,7 +842,7 @@ void ChromeLauncherController::PersistPinnedState() {
   // Mutating kPinnedLauncherApps is going to notify us and trigger us to
   // process the change. We don't want that to happen so remove ourselves as a
   // listener.
-  pref_change_registrar_.Remove(prefs::kPinnedLauncherApps, this);
+  pref_change_registrar_.Remove(prefs::kPinnedLauncherApps);
   {
     ListPrefUpdate updater(profile_->GetPrefs(), prefs::kPinnedLauncherApps);
     updater->Clear();

chrome/browser/ui/webui/options/core_options_handler.cc

@@ -206,9 +206,9 @@ void CoreOptionsHandler::ObservePref(const std::string& pref_name) {
 
 void CoreOptionsHandler::StopObservingPref(const std::string& pref_name) {
   if (g_browser_process->local_state()->FindPreference(pref_name.c_str()))
-    local_state_registrar_.Remove(pref_name.c_str(), this);
+    local_state_registrar_.Remove(pref_name.c_str());
   else
-    registrar_.Remove(pref_name.c_str(), this);
+    registrar_.Remove(pref_name.c_str());
 }
 
 void CoreOptionsHandler::SetPref(const std::string& pref_name,