This is a redo of my previous notification registrar change. I saw a crash in handling NAV_ENTRY_COMMITTED, so am changing all consumers of this to use the registrar so that it is impossible to forget to unregister.

The difference is that in tab_contents I moved the removal code in RemoveInfoBar to only remove the listener if an infobar was actually removed.
Review URL: http://codereview.chromium.org/16534

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

8 files changed, 64 insertions, 89 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index ff96f55f0..ec31959 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -29,6 +29,7 @@
 #include "chrome/browser/views/bookmark_bar_view.h"
 #include "chrome/browser/views/location_bar_view.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/pref_service.h"
 #include "chrome/test/automation/automation_messages.h"
 #include "chrome/views/app_modal_dialog_delegate.h"
@@ -45,30 +46,21 @@ class InitialLoadObserver : public NotificationObserver {
         automation_(automation) {
     if (outstanding_tab_count_ > 0) {
       NotificationService* service = NotificationService::current();
-      service->AddObserver(this, NOTIFY_LOAD_START,
-                           NotificationService::AllSources());
-      service->AddObserver(this, NOTIFY_LOAD_STOP,
-                          NotificationService::AllSources());
+      registrar_.Add(this, NOTIFY_LOAD_START,
+                     NotificationService::AllSources());
+      registrar_.Add(this, NOTIFY_LOAD_STOP,
+                     NotificationService::AllSources());
     }
   }
 
   ~InitialLoadObserver() {
-    Unregister();
   }
 
   void ConditionMet() {
-    Unregister();
+    registrar_.RemoveAll();
     automation_->Send(new AutomationMsg_InitialLoadsComplete(0));
   }
 
-  void Unregister() {
-    NotificationService* service = NotificationService::current();
-    service->RemoveObserver(this, NOTIFY_LOAD_START,
-                            NotificationService::AllSources());
-    service->RemoveObserver(this, NOTIFY_LOAD_STOP,
-                            NotificationService::AllSources());
-  }
-
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details) {
@@ -90,6 +82,8 @@ class InitialLoadObserver : public NotificationObserver {
  private:
   typedef std::set<uintptr_t> TabSet;
 
+  NotificationRegistrar registrar_;
+
   AutomationProvider* automation_;
   size_t outstanding_tab_count_;
   TabSet loading_tabs_;
diff --git a/chrome/browser/automation/automation_tab_tracker.h b/chrome/browser/automation/automation_tab_tracker.h
index 9e0c63c..1033ad0 100644
--- a/chrome/browser/automation/automation_tab_tracker.h
+++ b/chrome/browser/automation/automation_tab_tracker.h
@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H__
-#define CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H__
+#ifndef CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H_
+#define CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H_
 
 #include "chrome/browser/automation/automation_resource_tracker.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/navigation_controller.h"
+#include "chrome/common/notification_registrar.h"
 
 class AutomationTabTracker
   : public AutomationResourceTracker<NavigationController*> {
@@ -22,28 +23,24 @@ public:
 
   virtual void AddObserver(NavigationController* resource) {
     // This tab could either be a regular tab or an external tab
-    // Register for both notifications
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_TAB_CLOSING, Source<NavigationController>(resource));
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_EXTERNAL_TAB_CLOSED,
-        Source<NavigationController>(resource));
+    // Register for both notifications.
+    registrar_.Add(this, NOTIFY_TAB_CLOSING,
+                   Source<NavigationController>(resource));
+    registrar_.Add(this, NOTIFY_EXTERNAL_TAB_CLOSED,
+                   Source<NavigationController>(resource));
     // We also want to know about navigations so we can keep track of the last
     // navigation time.
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(resource));
+    registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                   Source<NavigationController>(resource));
   }
 
   virtual void RemoveObserver(NavigationController* resource) {
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_TAB_CLOSING, Source<NavigationController>(resource));
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_EXTERNAL_TAB_CLOSED,
-        Source<NavigationController>(resource));
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(resource));
+    registrar_.Remove(this, NOTIFY_TAB_CLOSING,
+                      Source<NavigationController>(resource));
+    registrar_.Remove(this, NOTIFY_EXTERNAL_TAB_CLOSED,
+                      Source<NavigationController>(resource));
+    registrar_.Remove(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                      Source<NavigationController>(resource));
   }
 
   virtual void Observe(NotificationType type,
@@ -80,11 +77,12 @@ public:
   }
 
  private:
+  NotificationRegistrar registrar_;
+
   // Last time a navigation occurred.
-   std::map<NavigationController*, base::Time> last_navigation_times_;
+  std::map<NavigationController*, base::Time> last_navigation_times_;
 
   DISALLOW_COPY_AND_ASSIGN(AutomationTabTracker);
 };
 
-#endif // CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H__
-
+#endif  // CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H_
diff --git a/chrome/browser/jsmessage_box_handler.cc b/chrome/browser/jsmessage_box_handler.cc
index b86920b..2a1d2fd 100644
--- a/chrome/browser/jsmessage_box_handler.cc
+++ b/chrome/browser/jsmessage_box_handler.cc
@@ -37,12 +37,6 @@ void JavascriptMessageBoxHandler::RunJavascriptMessageBox(
 }
 
 JavascriptMessageBoxHandler::~JavascriptMessageBoxHandler() {
-  NotificationService::current()->
-      RemoveObserver(this, NOTIFY_NAV_ENTRY_COMMITTED,
-                     NotificationService::AllSources());
-  NotificationService::current()->
-      RemoveObserver(this, NOTIFY_TAB_CONTENTS_DESTROYED,
-                     NotificationService::AllSources());
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -209,11 +203,8 @@ JavascriptMessageBoxHandler::JavascriptMessageBoxHandler(
 
   // Make sure we get navigation notifications so we know when our parent
   // contents will disappear or navigate to a different page.
-  NotificationService::current()->
-      AddObserver(this, NOTIFY_NAV_ENTRY_COMMITTED,
-                  NotificationService::AllSources());
-  NotificationService::current()->
-      AddObserver(this, NOTIFY_TAB_CONTENTS_DESTROYED,
-                  NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_TAB_CONTENTS_DESTROYED,
+                 NotificationService::AllSources());
 }
-
diff --git a/chrome/browser/jsmessage_box_handler.h b/chrome/browser/jsmessage_box_handler.h
index 6873605..920d03d 100644
--- a/chrome/browser/jsmessage_box_handler.h
+++ b/chrome/browser/jsmessage_box_handler.h
@@ -2,12 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H__
-#define CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H__
+#ifndef CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
+#define CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
 
 #include "chrome/common/ipc_message.h"
 #include "chrome/views/app_modal_dialog_delegate.h"
 #include "chrome/common/notification_service.h"
+#include "chrome/common/notification_registrar.h"
 
 class MessageBoxView;
 class WebContents;
@@ -64,6 +65,8 @@ class JavascriptMessageBoxHandler
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
+  NotificationRegistrar registrar_;
+
   // The message box view whose commands we handle.
   MessageBoxView* message_box_view_;
 
@@ -83,5 +86,4 @@ class JavascriptMessageBoxHandler
   DISALLOW_EVIL_CONSTRUCTORS(JavascriptMessageBoxHandler);
 };
 
-#endif // CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H__
-
+#endif // CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index 6016651..227c6ff 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -126,20 +126,6 @@ SessionService::SessionService(const std::wstring& save_path)
 
 SessionService::~SessionService() {
   Save();
-
-  // Unregister our notifications.
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_TAB_PARENTED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_TAB_CLOSED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_LIST_PRUNED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_ENTRY_CHANGED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_ENTRY_COMMITTED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_BROWSER_OPENED, NotificationService::AllSources());
 }
 
 void SessionService::ResetFromCurrentBrowsers() {
@@ -380,18 +366,18 @@ SessionService::Handle SessionService::GetLastSession(
 
 void SessionService::Init() {
   // Register for the notifications we're interested in.
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_TAB_PARENTED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_TAB_CLOSED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_LIST_PRUNED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_ENTRY_CHANGED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_ENTRY_COMMITTED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_BROWSER_OPENED, NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_TAB_PARENTED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_TAB_CLOSED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_LIST_PRUNED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_CHANGED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_BROWSER_OPENED,
+                 NotificationService::AllSources());
 }
 
 void SessionService::Observe(NotificationType type,
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h
index 1c3402d3..7af43f0 100644
--- a/chrome/browser/sessions/session_service.h
+++ b/chrome/browser/sessions/session_service.h
@@ -12,6 +12,7 @@
 #include "chrome/browser/sessions/base_session_service.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/notification_service.h"
+#include "chrome/common/notification_registrar.h"
 
 class Browser;
 class NavigationController;
@@ -305,6 +306,8 @@ class SessionService : public BaseSessionService,
     return type == Browser::TYPE_NORMAL;
   }
 
+  NotificationRegistrar registrar_;
+
   // Maps from session tab id to the range of navigation entries that has
   // been written to disk.
   //
diff --git a/chrome/browser/tab_contents.cc b/chrome/browser/tab_contents.cc
index 8e93672..66774c6 100644
--- a/chrome/browser/tab_contents.cc
+++ b/chrome/browser/tab_contents.cc
@@ -66,7 +66,6 @@ void TabContents::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kBlockPopups, false);
 }
 
-
 void TabContents::CloseContents() {
   // Destroy our NavigationController, which will Destroy all tabs it owns.
   controller_->Destroy();
@@ -410,9 +409,9 @@ void TabContents::AddInfoBar(InfoBarDelegate* delegate) {
   // added. We use this notification to expire InfoBars that need to expire on
   // page transitions.
   if (infobar_delegates_.size() == 1) {
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(controller()));
+    DCHECK(controller());
+    registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                   Source<NavigationController>(controller()));
   }
 }
 
@@ -425,13 +424,12 @@ void TabContents::RemoveInfoBar(InfoBarDelegate* delegate) {
                                            Source<TabContents>(this),
                                            Details<InfoBarDelegate>(delegate));
     infobar_delegates_.erase(it);
-  }
 
-  // Remove ourselves as an observer if we are tracking no more InfoBars.
-  if (infobar_delegates_.empty()) {
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(controller()));
+    // Remove ourselves as an observer if we are tracking no more InfoBars.
+    if (infobar_delegates_.empty()) {
+      registrar_.Remove(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                        Source<NavigationController>(controller()));
+    }
   }
 }
 
diff --git a/chrome/browser/tab_contents.h b/chrome/browser/tab_contents.h
index 0c40e36..9d96d60 100644
--- a/chrome/browser/tab_contents.h
+++ b/chrome/browser/tab_contents.h
@@ -15,6 +15,7 @@
 #include "chrome/browser/page_navigator.h"
 #include "chrome/browser/tab_contents_type.h"
 #include "chrome/common/navigation_types.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/property_bag.h"
 
 namespace gfx {
@@ -511,6 +512,8 @@ class TabContents : public PageNavigator,
 
   PropertyBag property_bag_;
 
+  NotificationRegistrar registrar_;
+
   // Indicates whether we're currently loading a resource.
   bool is_loading_;