The last NotificationRegistrar patch: fix the unittests to use a registrar, and turn off access to AddObserver()/RemoveObserver() entirely.

This eliminates one element of the unittests which tested that removing an observer that doesn't exist is harmless; in NotificationRegistrar-land, doing this will NOTREACHED().

BUG=2381
Review URL: http://codereview.chromium.org/114044

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

2 files changed, 55 insertions, 75 deletions

diff --git a/chrome/common/notification_service.h b/chrome/common/notification_service.h
index 0f99949..fefc09c 100644
--- a/chrome/common/notification_service.h
+++ b/chrome/common/notification_service.h
@@ -30,36 +30,6 @@ class NotificationService {
   NotificationService();
   ~NotificationService();
 
-  // NOTE: Rather than using this directly, you you probably want to use a
-  // NotificationRegistrar.  It's generally easier and less error-prone.
-  //
-  // Registers a NotificationObserver to be called whenever a matching
-  // notification is posted.  Observer is a pointer to an object subclassing
-  // NotificationObserver to be notified when an event matching the other two
-  // parameters is posted to this service.  Type is the type of events to
-  // be notified about (or NOTIFY_ALL to receive events of all types).
-  // Source is a NotificationSource object (created using
-  // "Source<classname>(pointer)"), if this observer only wants to
-  // receive events from that object, or NotificationService::AllSources()
-  // to receive events from all sources.
-  //
-  // A given observer can be registered only once for each combination of
-  // type and source.  If the same object is registered more than once,
-  // it must be removed for each of those combinations of type and source later.
-  //
-  // The caller retains ownership of the object pointed to by observer.
-  void AddObserver(NotificationObserver* observer,
-                   NotificationType type, const NotificationSource& source);
-
-  // NOTE: Rather than using this directly, you you probably want to use a
-  // NotificationRegistrar.  It's generally easier and less error-prone.
-  //
-  // Removes the object pointed to by observer from receiving notifications
-  // that match type and source.  If no object matching the parameters is
-  // currently registered, this method is a no-op.
-  void RemoveObserver(NotificationObserver* observer,
-                      NotificationType type, const NotificationSource& source);
-
   // Synchronously posts a notification to all interested observers.
   // Source is a reference to a NotificationSource object representing
   // the object originating the notification (can be
@@ -81,6 +51,8 @@ class NotificationService {
   static Details<void> NoDetails() { return Details<void>(NULL); }
 
  private:
+  friend class NotificationRegistrar;
+
   typedef ObserverList<NotificationObserver> NotificationObserverList;
   typedef std::map<uintptr_t, NotificationObserverList*> NotificationSourceMap;
 
@@ -89,6 +61,36 @@ class NotificationService {
   static bool HasKey(const NotificationSourceMap& map,
                      const NotificationSource& source);
 
+  // NOTE: Rather than using this directly, you should use a
+  // NotificationRegistrar.
+  //
+  // Registers a NotificationObserver to be called whenever a matching
+  // notification is posted.  Observer is a pointer to an object subclassing
+  // NotificationObserver to be notified when an event matching the other two
+  // parameters is posted to this service.  Type is the type of events to
+  // be notified about (or NOTIFY_ALL to receive events of all types).
+  // Source is a NotificationSource object (created using
+  // "Source<classname>(pointer)"), if this observer only wants to
+  // receive events from that object, or NotificationService::AllSources()
+  // to receive events from all sources.
+  //
+  // A given observer can be registered only once for each combination of
+  // type and source.  If the same object is registered more than once,
+  // it must be removed for each of those combinations of type and source later.
+  //
+  // The caller retains ownership of the object pointed to by observer.
+  void AddObserver(NotificationObserver* observer,
+                   NotificationType type, const NotificationSource& source);
+
+  // NOTE: Rather than using this directly, you should use a
+  // NotificationRegistrar.
+  //
+  // Removes the object pointed to by observer from receiving notifications
+  // that match type and source.  If no object matching the parameters is
+  // currently registered, this method is a no-op.
+  void RemoveObserver(NotificationObserver* observer,
+                      NotificationType type, const NotificationSource& source);
+
   // Keeps track of the observers for each type of notification.
   // Until we get a prohibitively large number of notification types,
   // a simple array is probably the fastest way to dispatch.
diff --git a/chrome/common/notification_service_unittest.cc b/chrome/common/notification_service_unittest.cc
index 4fed35f..d22b162 100644
--- a/chrome/common/notification_service_unittest.cc
+++ b/chrome/common/notification_service_unittest.cc
@@ -2,12 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 class NotificationServiceTest: public testing::Test {
+ protected:
+  NotificationRegistrar registrar_;
 };
 
 class TestObserver : public NotificationObserver {
@@ -32,7 +35,7 @@ class TestSource {};
 }  // namespace
 
 
-TEST(NotificationServiceTest, Basic) {
+TEST_F(NotificationServiceTest, Basic) {
   TestSource test_source;
   TestSource other_source;
 
@@ -47,25 +50,20 @@ TEST(NotificationServiceTest, Basic) {
   TestObserver all_types_test_source;
   TestObserver idle_test_source;
 
-  NotificationService* service = NotificationService::current();
-
   // Make sure it doesn't freak out when there are no observers.
+  NotificationService* service = NotificationService::current();
   service->Notify(NotificationType::IDLE,
                   Source<TestSource>(&test_source),
                   NotificationService::NoDetails());
 
-  service->AddObserver(&all_types_all_sources,
-                       NotificationType::ALL,
-                       NotificationService::AllSources());
-  service->AddObserver(&idle_all_sources,
-                       NotificationType::IDLE,
-                       NotificationService::AllSources());
-  service->AddObserver(&all_types_test_source,
-                       NotificationType::ALL,
-                       Source<TestSource>(&test_source));
-  service->AddObserver(&idle_test_source,
-                       NotificationType::IDLE,
-                       Source<TestSource>(&test_source));
+  registrar_.Add(&all_types_all_sources, NotificationType::ALL,
+                 NotificationService::AllSources());
+  registrar_.Add(&idle_all_sources, NotificationType::IDLE,
+                 NotificationService::AllSources());
+  registrar_.Add(&all_types_test_source, NotificationType::ALL,
+                 Source<TestSource>(&test_source));
+  registrar_.Add(&idle_test_source, NotificationType::IDLE,
+                 Source<TestSource>(&test_source));
 
   EXPECT_EQ(0, all_types_all_sources.notification_count());
   EXPECT_EQ(0, idle_all_sources.notification_count());
@@ -118,18 +116,7 @@ TEST(NotificationServiceTest, Basic) {
   EXPECT_EQ(2, all_types_test_source.notification_count());
   EXPECT_EQ(1, idle_test_source.notification_count());
 
-  service->RemoveObserver(&all_types_all_sources,
-                          NotificationType::ALL,
-                          NotificationService::AllSources());
-  service->RemoveObserver(&idle_all_sources,
-                          NotificationType::IDLE,
-                          NotificationService::AllSources());
-  service->RemoveObserver(&all_types_test_source,
-                          NotificationType::ALL,
-                          Source<TestSource>(&test_source));
-  service->RemoveObserver(&idle_test_source,
-                          NotificationType::IDLE,
-                          Source<TestSource>(&test_source));
+  registrar_.RemoveAll();
 
   service->Notify(NotificationType::IDLE,
                   Source<TestSource>(&test_source),
@@ -139,44 +126,35 @@ TEST(NotificationServiceTest, Basic) {
   EXPECT_EQ(3, idle_all_sources.notification_count());
   EXPECT_EQ(2, all_types_test_source.notification_count());
   EXPECT_EQ(1, idle_test_source.notification_count());
-
-  // Removing an observer that isn't there is a no-op, this should be fine.
-  service->RemoveObserver(&all_types_all_sources,
-                          NotificationType::ALL,
-                          NotificationService::AllSources());
 }
 
-TEST(NotificationServiceTest, MultipleRegistration) {
+TEST_F(NotificationServiceTest, MultipleRegistration) {
   TestSource test_source;
 
   TestObserver idle_test_source;
 
   NotificationService* service = NotificationService::current();
 
-  service->AddObserver(&idle_test_source,
-                       NotificationType::IDLE,
-                       Source<TestSource>(&test_source));
-  service->AddObserver(&idle_test_source,
-                       NotificationType::ALL,
-                       Source<TestSource>(&test_source));
+  registrar_.Add(&idle_test_source, NotificationType::IDLE,
+                 Source<TestSource>(&test_source));
+  registrar_.Add(&idle_test_source, NotificationType::ALL,
+                 Source<TestSource>(&test_source));
 
   service->Notify(NotificationType::IDLE,
                   Source<TestSource>(&test_source),
                   NotificationService::NoDetails());
   EXPECT_EQ(2, idle_test_source.notification_count());
 
-  service->RemoveObserver(&idle_test_source,
-                          NotificationType::IDLE,
-                          Source<TestSource>(&test_source));
+  registrar_.Remove(&idle_test_source, NotificationType::IDLE,
+                    Source<TestSource>(&test_source));
 
   service->Notify(NotificationType::IDLE,
                  Source<TestSource>(&test_source),
                  NotificationService::NoDetails());
   EXPECT_EQ(3, idle_test_source.notification_count());
 
-  service->RemoveObserver(&idle_test_source,
-                          NotificationType::ALL,
-                          Source<TestSource>(&test_source));
+  registrar_.Remove(&idle_test_source, NotificationType::ALL,
+                    Source<TestSource>(&test_source));
 
   service->Notify(NotificationType::IDLE,
                   Source<TestSource>(&test_source),