Remove the rest of the alternate nav url fetcher from the navigation controller.

This changes the memory model around a bit, and it's not the most clear thing
ever, not that it was before. The alternate URL fetcher is now responsible for
deleting itself in most cases.

BUG=2370 (Assertion when using the alternate URL tracker twice in a row)
BUG=1324500 (Move the AlternateNavURLFetcher logic out of NavigationController)
Review URL: http://codereview.chromium.org/2905

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

4 files changed, 27 insertions, 40 deletions

diff --git a/chrome/browser/alternate_nav_url_fetcher.cc b/chrome/browser/alternate_nav_url_fetcher.cc
index 3f16e2f..9560d9b 100644
--- a/chrome/browser/alternate_nav_url_fetcher.cc
+++ b/chrome/browser/alternate_nav_url_fetcher.cc
@@ -19,25 +19,23 @@ AlternateNavURLFetcher::AlternateNavURLFetcher(
                  NotificationService::AllSources());
 }
 
-AlternateNavURLFetcher::~AlternateNavURLFetcher() {
-}
-
 void AlternateNavURLFetcher::Observe(NotificationType type,
                                      const NotificationSource& source,
                                      const NotificationDetails& details) {
   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);
+      DCHECK(controller_->GetPendingEntry());
 
       // Unregister for this notification now that we're pending, and start
-      // listening for the corresponding commit.
+      // listening for the corresponding commit. We also need to listen for the
+      // tab close command since that means the load will never commit!
       registrar_.Remove(this, NOTIFY_NAV_ENTRY_PENDING,
                         NotificationService::AllSources());
       registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
                      Source<NavigationController>(controller_));
+      registrar_.Add(this, NOTIFY_TAB_CLOSED,
+                     Source<NavigationController>(controller_));
 
       DCHECK_EQ(NOT_STARTED, state_);
       state_ = IN_PROGRESS;
@@ -53,6 +51,14 @@ void AlternateNavURLFetcher::Observe(NotificationType type,
                         Source<NavigationController>(controller_));
       navigated_to_entry_ = true;
       ShowInfobarIfPossible();
+      // DON'T DO ANYTHING AFTER HERE SINCE |this| MAY BE DELETED!
+      break;
+
+    case NOTIFY_TAB_CLOSED:
+      // We listen either for tab closed or navigation committed to know when to
+      // delete ourselves. Here, the tab closed, so we can just give up with
+      // all our waiting for notifications and delete ourselves.
+      delete this;
       break;
 
     default:
@@ -73,6 +79,7 @@ void AlternateNavURLFetcher::OnURLFetchComplete(const URLFetcher* source,
        (response_code == 401) || (response_code == 407))) {
     state_ = SUCCEEDED;
     ShowInfobarIfPossible();
+    // DON'T DO ANYTHING AFTER HERE SINCE |this| MAY BE DELETED!
   } else {
     state_ = FAILED;
   }
@@ -94,5 +101,9 @@ void AlternateNavURLFetcher::ShowInfobarIfPossible() {
   // navigation, so we don't need to keep track of it.
   web_contents->GetInfoBarView()->AddChildView(new InfoBarAlternateNavURLView(
       alternate_nav_url_));
+
+  // Now we're no longer referencing the navigation controller or the url fetch,
+  // so our job is done.
+  delete this;
 }
 
diff --git a/chrome/browser/alternate_nav_url_fetcher.h b/chrome/browser/alternate_nav_url_fetcher.h
index 94833a4..b28b48f0 100644
--- a/chrome/browser/alternate_nav_url_fetcher.h
+++ b/chrome/browser/alternate_nav_url_fetcher.h
@@ -18,6 +18,13 @@ class NavigationController;
 // tell if the entry was a search or an intranet hostname. The autocomplete bar
 // assumes it's a query and issues an AlternateNavURLFetcher to display a "did
 // you mean" infobar suggesting a navigation.
+//
+// The memory management of this object is a bit tricky. The location bar view
+// will create us and be responsible for us until we attach as an observer
+// after a pending load starts (it will delete us if this doesn't happen).
+// Once this pending load starts, we're responsible for deleting ourselves.
+// We'll do this when the load commits, or when the navigation controller
+// itself is deleted.
 class AlternateNavURLFetcher : public NotificationObserver,
                                public URLFetcher::Delegate {
  public:
@@ -29,7 +36,6 @@ class AlternateNavURLFetcher : public NotificationObserver,
   };
 
   explicit AlternateNavURLFetcher(const std::wstring& alternate_nav_url);
-  virtual ~AlternateNavURLFetcher();
 
   State state() const { return state_; }
 
@@ -48,7 +54,8 @@ class AlternateNavURLFetcher : public NotificationObserver,
 
  private:
   // Displays the infobar if all conditions are met (the page has loaded and
-  // the fetch of the alternate URL succeeded).
+  // the fetch of the alternate URL succeeded). If the infobar is displayed,
+  // this object is no longer necessary and this function WILL DELETE |this|!.
   void ShowInfobarIfPossible();
 
   std::wstring alternate_nav_url_;
diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index e7e9aa9..a0dab1b 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -166,7 +166,6 @@ NavigationController::NavigationController(TabContents* contents,
       pending_entry_index_(-1),
       max_entry_count_(kMaxEntryCount),
       active_contents_(contents),
-      alternate_nav_url_fetcher_entry_unique_id_(0),
       max_restored_page_id_(-1),
       ssl_manager_(this, NULL),
       needs_reload_(false),
@@ -187,7 +186,6 @@ NavigationController::NavigationController(
       pending_entry_index_(-1),
       max_entry_count_(kMaxEntryCount),
       active_contents_(NULL),
-      alternate_nav_url_fetcher_entry_unique_id_(0),
       max_restored_page_id_(-1),
       ssl_manager_(this, NULL),
       needs_reload_(true),
@@ -488,14 +486,6 @@ const SkBitmap& NavigationController::GetLazyFavIcon() const {
   }
 }
 
-void NavigationController::SetAlternateNavURLFetcher(
-    AlternateNavURLFetcher* alternate_nav_url_fetcher) {
-  DCHECK(!alternate_nav_url_fetcher_.get());
-  DCHECK(pending_entry_);
-  alternate_nav_url_fetcher_.reset(alternate_nav_url_fetcher);
-  alternate_nav_url_fetcher_entry_unique_id_ = pending_entry_->unique_id();
-}
-
 bool NavigationController::RendererDidNavigate(
     const ViewHostMsg_FrameNavigate_Params& params,
     bool is_interstitial,
@@ -1013,18 +1003,6 @@ void NavigationController::NavigateToPendingEntry(bool reload) {
 
 void NavigationController::NotifyNavigationEntryCommitted(
     LoadCommittedDetails* details) {
-  // Reset the Alternate Nav URL Fetcher if we're loading some page it doesn't
-  // care about.  We must do this before calling Notify() below as that may
-  // result in the creation of a new fetcher.
-  //
-  // TODO(brettw) bug 1324500: this logic should be moved out of the controller!
-  const NavigationEntry* const entry = GetActiveEntry();
-  if (!entry ||
-      (entry->unique_id() != alternate_nav_url_fetcher_entry_unique_id_)) {
-    alternate_nav_url_fetcher_.reset();
-    alternate_nav_url_fetcher_entry_unique_id_ = 0;
-  }
-
   // TODO(pkasting): http://b/1113079 Probably these explicit notification paths
   // should be removed, and interested parties should just listen for the
   // notification below instead.
diff --git a/chrome/browser/navigation_controller.h b/chrome/browser/navigation_controller.h
index f099704..3380732f 100644
--- a/chrome/browser/navigation_controller.h
+++ b/chrome/browser/navigation_controller.h
@@ -9,7 +9,6 @@
 
 #include "base/linked_ptr.h"
 #include "base/ref_counted.h"
-#include "chrome/browser/alternate_nav_url_fetcher.h"
 #include "chrome/browser/session_service.h"
 #include "chrome/browser/site_instance.h"
 #include "chrome/browser/ssl_manager.h"
@@ -311,10 +310,6 @@ class NavigationController {
   const std::wstring& GetLazyTitle() const;
   const SkBitmap& GetLazyFavIcon() const;
 
-  // TODO(brettw) bug 1324500: move this out of here.
-  void SetAlternateNavURLFetcher(
-      AlternateNavURLFetcher* alternate_nav_url_fetcher);
-
   // Returns the identifier used by session restore.
   const SessionID& session_id() const { return session_id_; }
 
@@ -512,10 +507,6 @@ class NavigationController {
   // The tab contents that is currently active.
   TabContents* active_contents_;
 
-  // The AlternateNavURLFetcher and its associated active entry, if any.
-  scoped_ptr<AlternateNavURLFetcher> alternate_nav_url_fetcher_;
-  int alternate_nav_url_fetcher_entry_unique_id_;
-
   // The max restored page ID in this controller, if it was restored.  We must
   // store this so that WebContents can tell any renderer in charge of one of
   // the restored entries to update its max page ID.