Remove an explicit call from the NavigationController to the alternate URL

fetcher since there is already a notification that does this.

I created a class that will automatically unregister for notifications when it
goes out of scope and used it here. I think it will be useful for most consumers
of notifications.
Review URL: http://codereview.chromium.org/2895

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

6 files changed, 177 insertions, 54 deletions

diff --git a/chrome/browser/alternate_nav_url_fetcher.cc b/chrome/browser/alternate_nav_url_fetcher.cc
index fef750e..3f16e2f 100644
--- a/chrome/browser/alternate_nav_url_fetcher.cc
+++ b/chrome/browser/alternate_nav_url_fetcher.cc
@@ -11,47 +11,53 @@
 
 AlternateNavURLFetcher::AlternateNavURLFetcher(
     const std::wstring& alternate_nav_url)
-  : alternate_nav_url_(alternate_nav_url),
-    controller_(NULL),
-    state_(NOT_STARTED),
-    navigated_to_entry_(false) {
-  NotificationService::current()->AddObserver(this,
-      NOTIFY_NAV_ENTRY_PENDING, NotificationService::AllSources());
+    : alternate_nav_url_(alternate_nav_url),
+      controller_(NULL), 
+      state_(NOT_STARTED),
+      navigated_to_entry_(false) {
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_PENDING,
+                 NotificationService::AllSources());
 }
 
 AlternateNavURLFetcher::~AlternateNavURLFetcher() {
-  if (state_ == NOT_STARTED) {
-    // Never caught the NavigationController notification.
-    NotificationService::current()->RemoveObserver(this,
-        NOTIFY_NAV_ENTRY_PENDING, NotificationService::AllSources());
-  }  // Otherwise, Observe() removed the observer already.
-}
-
-void AlternateNavURLFetcher::OnNavigatedToEntry() {
-  if (navigated_to_entry_)
-    return;
-  navigated_to_entry_ = true;
-  if (state_ == SUCCEEDED)
-    ShowInfobar();
 }
 
 void AlternateNavURLFetcher::Observe(NotificationType type,
                                      const NotificationSource& source,
                                      const NotificationDetails& details) {
-  controller_ = Source<NavigationController>(source).ptr();
-  if (!controller_->GetPendingEntry())
-    return;  // No entry to attach ourselves to.
-  controller_->SetAlternateNavURLFetcher(this);
+  switch (type) {
+    case NOTIFY_NAV_ENTRY_PENDING:
+      controller_ = Source<NavigationController>(source).ptr();
+      if (!controller_->GetPendingEntry())
+        return;  // No entry to attach ourselves to.
+      controller_->SetAlternateNavURLFetcher(this);
+
+      // Unregister for this notification now that we're pending, and start
+      // listening for the corresponding commit.
+      registrar_.Remove(this, NOTIFY_NAV_ENTRY_PENDING,
+                        NotificationService::AllSources());
+      registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                     Source<NavigationController>(controller_));
 
-  NotificationService::current()->RemoveObserver(this,
-      NOTIFY_NAV_ENTRY_PENDING, NotificationService::AllSources());
+      DCHECK_EQ(NOT_STARTED, state_);
+      state_ = IN_PROGRESS;
+      fetcher_.reset(new URLFetcher(GURL(alternate_nav_url_), URLFetcher::HEAD,
+                                    this));
+      fetcher_->set_request_context(controller_->profile()->GetRequestContext());
+      fetcher_->Start();
+      break;
 
-  DCHECK_EQ(NOT_STARTED, state_);
-  state_ = IN_PROGRESS;
-  fetcher_.reset(new URLFetcher(GURL(alternate_nav_url_), URLFetcher::HEAD,
-                                this));
-  fetcher_->set_request_context(controller_->profile()->GetRequestContext());
-  fetcher_->Start();
+    case NOTIFY_NAV_ENTRY_COMMITTED:
+      // The page was navigated, we can show the infobar now if necessary.
+      registrar_.Remove(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                        Source<NavigationController>(controller_));
+      navigated_to_entry_ = true;
+      ShowInfobarIfPossible();
+      break;
+
+    default:
+      NOTREACHED();
+  }
 }
 
 void AlternateNavURLFetcher::OnURLFetchComplete(const URLFetcher* source,
@@ -66,14 +72,16 @@ void AlternateNavURLFetcher::OnURLFetchComplete(const URLFetcher* source,
       (((response_code / 100) == 2) ||
        (response_code == 401) || (response_code == 407))) {
     state_ = SUCCEEDED;
-    if (navigated_to_entry_)
-      ShowInfobar();
+    ShowInfobarIfPossible();
   } else {
     state_ = FAILED;
   }
 }
 
-void AlternateNavURLFetcher::ShowInfobar() {
+void AlternateNavURLFetcher::ShowInfobarIfPossible() {
+  if (!navigated_to_entry_ || state_ != SUCCEEDED)
+    return;
+
   const NavigationEntry* const entry = controller_->GetActiveEntry();
   DCHECK(entry);
   if (entry->tab_type() != TAB_CONTENTS_WEB)
diff --git a/chrome/browser/alternate_nav_url_fetcher.h b/chrome/browser/alternate_nav_url_fetcher.h
index b259a64..94833a4 100644
--- a/chrome/browser/alternate_nav_url_fetcher.h
+++ b/chrome/browser/alternate_nav_url_fetcher.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_ALTERNATE_NAV_URL_FETCHER_H__
-#define CHROME_BROWSER_ALTERNATE_NAV_URL_FETCHER_H__
+#ifndef CHROME_BROWSER_ALTERNATE_NAV_URL_FETCHER_H_
+#define CHROME_BROWSER_ALTERNATE_NAV_URL_FETCHER_H_
 
 #include <string>
 
 #include "chrome/browser/url_fetcher.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 
 class NavigationController;
@@ -32,12 +33,6 @@ class AlternateNavURLFetcher : public NotificationObserver,
 
   State state() const { return state_; }
 
-  // Called by the NavigationController when it successfully navigates to the
-  // entry for which the fetcher is looking up an alternative.
-  // NOTE: This can be theoretically called multiple times, if multiple
-  // navigations with the same unique ID succeed.
-  void OnNavigatedToEntry();
-
   // NotificationObserver
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
@@ -52,7 +47,9 @@ class AlternateNavURLFetcher : public NotificationObserver,
                                   const std::string& data);
 
  private:
-  void ShowInfobar();
+  // Displays the infobar if all conditions are met (the page has loaded and
+  // the fetch of the alternate URL succeeded).
+  void ShowInfobarIfPossible();
 
   std::wstring alternate_nav_url_;
   scoped_ptr<URLFetcher> fetcher_;
@@ -60,8 +57,10 @@ class AlternateNavURLFetcher : public NotificationObserver,
   State state_;
   bool navigated_to_entry_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(AlternateNavURLFetcher);
+  NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(AlternateNavURLFetcher);
 };
 
-#endif  // CHROME_BROWSER_ALTERNATE_NAV_URL_FETCHER_H__
+#endif  // CHROME_BROWSER_ALTERNATE_NAV_URL_FETCHER_H_
 
diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 6f723b4..e7e9aa9 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -569,15 +569,6 @@ bool NavigationController::RendererDidNavigate(
   details->is_main_frame = PageTransition::IsMainFrame(params.transition);
   NotifyNavigationEntryCommitted(details);
 
-  // Because this call may synchronously show an infobar, we do it last, to
-  // make sure all other state is stable and the infobar won't get blown away
-  // by some transition.
-  //
-  // TODO(brettw) bug 1324500: This logic should be moved out of here, it should
-  // listen for the notification instead.
-  if (alternate_nav_url_fetcher_.get())
-    alternate_nav_url_fetcher_->OnNavigatedToEntry();
-
   // Broadcast the NOTIFY_FRAME_PROVISIONAL_LOAD_COMMITTED notification for use
   // by the SSL manager.
   //
diff --git a/chrome/common/common.vcproj b/chrome/common/common.vcproj
index 3cd2809..a09fcba 100644
--- a/chrome/common/common.vcproj
+++ b/chrome/common/common.vcproj
@@ -510,6 +510,14 @@
 			>
 		</File>
 		<File
+			RelativePath=".\notification_registrar.cc"
+			>
+		</File>
+		<File
+			RelativePath=".\notification_registrar.h"
+			>
+		</File>
+		<File
 			RelativePath=".\notification_service.cc"
 			>
 		</File>
diff --git a/chrome/common/notification_registrar.cc b/chrome/common/notification_registrar.cc
new file mode 100644
index 0000000..fdedde4
--- /dev/null
+++ b/chrome/common/notification_registrar.cc
@@ -0,0 +1,54 @@
+// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+
+#include "chrome/common/notification_registrar.h"
+
+NotificationRegistrar::NotificationRegistrar() {
+}
+
+NotificationRegistrar::~NotificationRegistrar() {
+  RemoveAll();
+}
+
+void NotificationRegistrar::Add(NotificationObserver* observer,
+                                NotificationType type,
+                                NotificationSource source) {
+  Record record = { observer, type, source };
+  DCHECK(std::find(registered_.begin(), registered_.end(), record) ==
+         registered_.end()) << "Duplicate registration.";
+  registered_.push_back(record);
+
+  NotificationService::current()->AddObserver(observer, type, source);
+}
+
+void NotificationRegistrar::Remove(NotificationObserver* observer,
+                                   NotificationType type,
+                                   NotificationSource source) {
+  Record record = { observer, type, source };
+  RecordVector::iterator found = std::find(
+      registered_.begin(), registered_.end(), record);
+  if (found != registered_.end()) {
+    registered_.erase(found);
+  } else {
+    // Fall through to passing the removal through to the notification service.
+    // If it really isn't registered, it will also assert and do nothing, but
+    // we might as well catch the case where the class is trying to unregister
+    // for something they registered without going through us.
+    NOTREACHED();
+  }
+
+  NotificationService::current()->RemoveObserver(observer, type, source);
+}
+
+void NotificationRegistrar::RemoveAll() {
+  NotificationService* service = NotificationService::current();
+  for (size_t i = 0; i < registered_.size(); i++) {
+    service->RemoveObserver(registered_[i].observer,
+                            registered_[i].type,
+                            registered_[i].source);
+  }
+  registered_.clear();
+}
diff --git a/chrome/common/notification_registrar.h b/chrome/common/notification_registrar.h
new file mode 100644
index 0000000..e253eb4
--- /dev/null
+++ b/chrome/common/notification_registrar.h
@@ -0,0 +1,63 @@
+// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_COMMON_NOTIFICATION_REGISTRAR_H_
+#define CHROME_COMMON_NOTIFICATION_REGISTRAR_H_
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "chrome/common/notification_service.h"
+
+// Aids in registering for notifications and ensures that all registered
+// notifications are unregistered when the class is destroyed.
+//
+// The intended use is that you make a NotificationRegistrar member in your
+// class and use it to register your notifications instead of going through the
+// notification service directly. It will automatically unregister them for
+// you.
+class NotificationRegistrar {
+ public:
+  // This class must not be derived from (we don't have a virtual destructor so
+  // it won't work). Instead, use it as a member in your class.
+  NotificationRegistrar();
+  ~NotificationRegistrar();
+
+  // Wrappers around NotificationService::[Add|Remove]Observer.
+  void Add(NotificationObserver* observer,
+           NotificationType type,
+           NotificationSource source);
+  void Remove(NotificationObserver* observer,
+              NotificationType type,
+              NotificationSource source);
+
+  // Unregisters all notifications.
+  void RemoveAll();
+
+ private:
+  struct Record {
+    bool operator==(const Record& other) const {
+      return observer == other.observer &&
+             type == other.type &&
+             source == other.source;
+    }
+
+    NotificationObserver* observer;
+    NotificationType type;
+    NotificationSource source;
+  };
+
+  // We keep registered notifications in a simple vector. This means we'll do
+  // brute-force searches when removing them individually, but individual
+  // removal is uncommon, and there will typically only be a couple of
+  // notifications anyway.
+  typedef std::vector<Record> RecordVector;
+
+  // Lists all notifications we're currently registered for.
+  RecordVector registered_;
+
+  DISALLOW_COPY_AND_ASSIGN(NotificationRegistrar);
+};
+
+#endif  // CHROME_COMMON_NOTIFICATION_REGISTRAR_H_