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

3 files changed, 52 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.
   //