More comprehensive handling of NOTIFICATION_NAV_ENTRY_PENDING for GoogleURLTracker.

chrome/browser/google/google_url_tracker.cc

Index: chrome/browser/google/google_url_tracker.cc
===================================================================
--- chrome/browser/google/google_url_tracker.cc	(revision 143533)
+++ chrome/browser/google/google_url_tracker.cc	(working copy)
@@ -23,6 +23,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
@@ -38,11 +39,10 @@
 
 GoogleURLTrackerInfoBarDelegate* CreateInfoBar(
     InfoBarTabHelper* infobar_helper,
-    const GURL& search_url,
     GoogleURLTracker* google_url_tracker,
     const GURL& new_google_url) {
-  return new GoogleURLTrackerInfoBarDelegate(infobar_helper, search_url,
-      google_url_tracker, new_google_url);
+  return new GoogleURLTrackerInfoBarDelegate(infobar_helper, google_url_tracker,
+                                             new_google_url);
 }
 
 string16 GetHost(const GURL& url) {
@@ -56,15 +56,14 @@
 
 GoogleURLTrackerInfoBarDelegate::GoogleURLTrackerInfoBarDelegate(
     InfoBarTabHelper* infobar_helper,
-    const GURL& search_url,
     GoogleURLTracker* google_url_tracker,
     const GURL& new_google_url)
     : ConfirmInfoBarDelegate(infobar_helper),
       map_key_(infobar_helper),
-      search_url_(search_url),
       google_url_tracker_(google_url_tracker),
       new_google_url_(new_google_url),
-      showing_(false) {
+      showing_(false),
+      pending_id_(0) {
 }
 
 bool GoogleURLTrackerInfoBarDelegate::Accept() {
@@ -92,30 +91,30 @@
   return false;
 }
 
+bool GoogleURLTrackerInfoBarDelegate::ShouldExpireInternal(
+    const content::LoadCommittedDetails& details) const {
+  int unique_id = details.entry->GetUniqueID();
+  return (unique_id != contents_unique_id()) && (unique_id != pending_id_);
+}
+
 void GoogleURLTrackerInfoBarDelegate::SetGoogleURL(const GURL& new_google_url) {
   DCHECK_EQ(GetHost(new_google_url_), GetHost(new_google_url));
   new_google_url_ = new_google_url;
 }
 
-void GoogleURLTrackerInfoBarDelegate::Show() {
-  showing_ = true;
-  owner()->AddInfoBar(this);  // May delete |this| on failure!
+void GoogleURLTrackerInfoBarDelegate::Show(const GURL& search_url) {
+  if (!owner())
+    return;
+  StoreActiveEntryUniqueID(owner());
+  search_url_ = search_url;
+  pending_id_ = 0;
+  if (!showing_) {
+    showing_ = true;
+    owner()->AddInfoBar(this);  // May delete |this| on failure!
+  }
 }
 
 void GoogleURLTrackerInfoBarDelegate::Close(bool redo_search) {
-  if (redo_search && owner()) {
-    // Re-do the user's search on the new domain.
-    url_canon::Replacements<char> replacements;
-    const std::string& host(new_google_url_.host());
-    replacements.SetHost(host.data(), url_parse::Component(0, host.length()));
-    GURL new_search_url(search_url_.ReplaceComponents(replacements));
-    if (new_search_url.is_valid()) {
-      content::OpenURLParams params(new_search_url, content::Referrer(),
-          CURRENT_TAB, content::PAGE_TRANSITION_GENERATED, false);
-      owner()->web_contents()->OpenURL(params);
-    }
-  }
-
   if (!showing_) {
     // We haven't been added to a tab, so just delete ourselves.
     delete this;
@@ -136,8 +135,24 @@
   // subsequently reached here due to GoogleURLTracker::CloseAllInfoBars().
   // This case will no longer happen once infobars are refactored to own their
   // delegates.
-  if (owner())
-    owner()->RemoveInfoBar(this);
+  if (!owner())
+    return;
+
+  if (redo_search) {
+    // Re-do the user's search on the new domain.
+    DCHECK(search_url_.is_valid());
+    url_canon::Replacements<char> replacements;
+    const std::string& host(new_google_url_.host());
+    replacements.SetHost(host.data(), url_parse::Component(0, host.length()));
+    GURL new_search_url(search_url_.ReplaceComponents(replacements));
+    if (new_search_url.is_valid()) {
+      content::OpenURLParams params(new_search_url, content::Referrer(),
+          CURRENT_TAB, content::PAGE_TRANSITION_GENERATED, false);
+      owner()->web_contents()->OpenURL(params);
+    }
+  }
+
+  owner()->RemoveInfoBar(this);
 }
 
 GoogleURLTrackerInfoBarDelegate::~GoogleURLTrackerInfoBarDelegate() {
@@ -160,6 +175,29 @@
 }
 
 
+// GoogleURLTracker::MapEntry -------------------------------------------------
+
+GoogleURLTracker::MapEntry::MapEntry()
+    : infobar(NULL),
+      navigation_controller_source(
+          content::Source<content::NavigationController>(NULL)),
+      tab_contents_source(content::Source<TabContents>(NULL)) {

[Ilya Sherman, 2012/06/26 05:58:14]

nit: Why bother with the initializer list if this code isn't reachable?

[Peter Kasting, 2012/06/26 17:38:31]

I originally omitted this, but it wouldn't compile, because NotificationSource
apparently doesn't have a null constructor.

I'll remove the |infobar| initializer and add a comment for the other two.

+  NOTREACHED();
+}
+
+GoogleURLTracker::MapEntry::MapEntry(
+    GoogleURLTrackerInfoBarDelegate* infobar,
+    const content::NotificationSource& navigation_controller_source,
+    const content::NotificationSource& tab_contents_source)
+    : infobar(infobar),
+      navigation_controller_source(navigation_controller_source),
+      tab_contents_source(tab_contents_source) {
+}
+
+GoogleURLTracker::MapEntry::~MapEntry() {
+}
+
+
 // GoogleURLTracker -----------------------------------------------------------
 
 const char GoogleURLTracker::kDefaultGoogleHomepage[] =
@@ -177,7 +215,8 @@
       in_startup_sleep_(true),
       already_fetched_(false),
       need_to_fetch_(false),
-      need_to_prompt_(false) {
+      need_to_prompt_(false),
+      search_committed_(false) {
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
 
   // Because this function can be called during startup, when kicking off a URL
@@ -299,7 +338,7 @@
     } else if (fetched_google_url_ != url) {
       for (InfoBarMap::iterator i(infobar_map_.begin());
            i != infobar_map_.end(); ++i)
-        i->second->SetGoogleURL(fetched_google_url_);
+        i->second.infobar->SetGoogleURL(fetched_google_url_);
     }
   }
 }
@@ -313,29 +352,25 @@
           content::Source<content::NavigationController>(source).ptr();
       TabContents* tab_contents =
           TabContents::FromWebContents(controller->GetWebContents());
-      OnNavigationPending(source,
-                          content::Source<TabContents>(tab_contents),
+      OnNavigationPending(source, content::Source<TabContents>(tab_contents),
                           tab_contents->infobar_tab_helper(),
-                          controller->GetPendingEntry()->GetURL());
+                          controller->GetPendingEntry()->GetUniqueID());
       break;
     }
 
     case content::NOTIFICATION_NAV_ENTRY_COMMITTED: {
-      TabContents* tab_contents = TabContents::FromWebContents(
-          content::Source<content::NavigationController>(source)->
-              GetWebContents());
-      OnNavigationCommittedOrTabClosed(source,
-          content::Source<TabContents>(tab_contents),
-          tab_contents->infobar_tab_helper(), true);
+      content::NavigationController* controller =
+          content::Source<content::NavigationController>(source).ptr();
+      OnNavigationCommittedOrTabClosed(
+          TabContents::FromWebContents(controller->GetWebContents())->
+              infobar_tab_helper(),
+          controller->GetActiveEntry()->GetURL());
       break;
     }
 
     case chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED: {
-      TabContents* tab_contents = content::Source<TabContents>(source).ptr();
       OnNavigationCommittedOrTabClosed(
-          content::Source<content::NavigationController>(
-              &tab_contents->web_contents()->GetController()), source,
-          tab_contents->infobar_tab_helper(), false);
+          content::Source<TabContents>(source)->infobar_tab_helper(), GURL());
       break;
     }
 
@@ -344,12 +379,10 @@
       content::WebContents* web_contents = tab_contents->web_contents();
       content::Source<content::NavigationController> source(
           &web_contents->GetController());
-      content::Source<TabContents> tab_contents_source(tab_contents);
       InfoBarTabHelper* infobar_helper = tab_contents->infobar_tab_helper();
-      OnNavigationPending(source, tab_contents_source, infobar_helper,
-                          web_contents->GetURL());
-      OnNavigationCommittedOrTabClosed(source, tab_contents_source,
-                                       infobar_helper, true);
+      OnNavigationPending(source, content::Source<TabContents>(tab_contents),
+                          infobar_helper, 0);
+      OnNavigationCommittedOrTabClosed(infobar_helper, web_contents->GetURL());
       break;
     }
 
@@ -393,9 +426,32 @@
 }
 
 void GoogleURLTracker::InfoBarClosed(const InfoBarTabHelper* infobar_helper) {
+  DCHECK(!search_committed_);
   InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
   DCHECK(i != infobar_map_.end());
+  const MapEntry& map_entry = i->second;
+
+  UnregisterForEntrySpecificNotifications(map_entry, false);
   infobar_map_.erase(i);
+
+  // Our global listeners for these other notifications should be in place iff
+  // we have any non-showing infobars.
+  for (InfoBarMap::const_iterator i(infobar_map_.begin());
+       i != infobar_map_.end(); ++i) {
+    if (!i->second.infobar->showing()) {
+      DCHECK(registrar_.IsRegistered(this,
+          content::NOTIFICATION_NAV_ENTRY_PENDING,
+          content::NotificationService::AllBrowserContextsAndSources()));
+      return;
+    }
+  }
+  if (registrar_.IsRegistered(this, content::NOTIFICATION_NAV_ENTRY_PENDING,
+      content::NotificationService::AllBrowserContextsAndSources())) {
+    registrar_.Remove(this, content::NOTIFICATION_NAV_ENTRY_PENDING,
+        content::NotificationService::AllBrowserContextsAndSources());
+    registrar_.Remove(this, chrome::NOTIFICATION_INSTANT_COMMITTED,
+        content::NotificationService::AllBrowserContextsAndSources());
+  }
 }
 
 void GoogleURLTracker::SetNeedToFetch() {
@@ -446,12 +502,16 @@
 
 void GoogleURLTracker::SearchCommitted() {
   if (need_to_prompt_) {
-    // This notification will fire a bit later in the same call chain we're
+    search_committed_ = true;
+    // These notifications will fire a bit later in the same call chain we're
     // currently in.
-    registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_PENDING,
-        content::NotificationService::AllBrowserContextsAndSources());
-    registrar_.Add(this, chrome::NOTIFICATION_INSTANT_COMMITTED,
-        content::NotificationService::AllBrowserContextsAndSources());
+    if (!registrar_.IsRegistered(this, content::NOTIFICATION_NAV_ENTRY_PENDING,
+        content::NotificationService::AllBrowserContextsAndSources())) {
+      registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_PENDING,
+          content::NotificationService::AllBrowserContextsAndSources());
+      registrar_.Add(this, chrome::NOTIFICATION_INSTANT_COMMITTED,
+          content::NotificationService::AllBrowserContextsAndSources());
+    }
   }
 }
 
@@ -459,60 +519,110 @@
     const content::NotificationSource& navigation_controller_source,
     const content::NotificationSource& tab_contents_source,
     InfoBarTabHelper* infobar_helper,
-    const GURL& search_url) {
-  registrar_.Remove(this, content::NOTIFICATION_NAV_ENTRY_PENDING,
-      content::NotificationService::AllBrowserContextsAndSources());
-  registrar_.Remove(this, chrome::NOTIFICATION_INSTANT_COMMITTED,
-      content::NotificationService::AllBrowserContextsAndSources());
+    int pending_id) {
+  InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
 
-  if (registrar_.IsRegistered(this,
-      chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED, tab_contents_source)) {
-    // If the previous load hasn't committed and the user triggers a new load,
-    // we don't need to re-register our listeners; just kill the old,
-    // never-shown infobar (to be replaced by a new one below).
-    InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
-    DCHECK(i != infobar_map_.end());
-    i->second->Close(false);
+  if (search_committed_) {
+    search_committed_ = false;
+    // Whether there's an existing infobar or not, we need to listen for the
+    // load to commit, so we can show and/or update the infobar when it does.
+    // (We may already be registered for this if there is an existing infobar
+    // that had a previous pending search that hasn't yet committed.)
+    if (!registrar_.IsRegistered(this,
+                                 content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+                                 navigation_controller_source)) {
+      registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+                      navigation_controller_source);
+    }
+    if (i == infobar_map_.end()) {
+      // This is a search on a tab that doesn't have one of our infobars, so add
+      // one.  Note that we only listen for the tab's destruction on this path;
+      // if there was already an infobar, then either it's not yet showing and
+      // we're already registered for this, or it is showing and its owner will
+      // handle tearing it down when the tab is destroyed.
+      registrar_.Add(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
+                     tab_contents_source);
+      infobar_map_.insert(std::make_pair(infobar_helper, MapEntry(
+          (*infobar_creator_)(infobar_helper, this, fetched_google_url_),
+          navigation_controller_source, tab_contents_source)));
+    } else {
+      // This is a new search on a tab where we already have an infobar (which
+      // may or may not be showing).
+      i->second.infobar->set_pending_id(pending_id);
+    }
+  } else if (i != infobar_map_.end()){
+    if (i->second.infobar->showing()) {
+      // This is a non-search navigation on a tab with a visible infobar.  If
+      // there was a previous pending search on this tab, this means it won't
+      // commit, so undo anything we did in response to seeing that.  Note that
+      // if there was no pending search on this tab, these statements are
+      // effectively a no-op.
+      //
+      // If this navigation actually commits, that will trigger the infobar's
+      // owner to expire the infobar if need be.  If it doesn't commit, then
+      // simply leaving the infobar as-is will have been the right thing.
+      UnregisterForEntrySpecificNotifications(i->second, false);
+      i->second.infobar->set_pending_id(0);
+    } else {
+      // Non-search navigation on a tab with a not-yet-shown infobar.  This
+      // means the original search won't commit, so close the infobar.
+      i->second.infobar->Close(false);
+    }
   } else {
-    // Start listening for the commit notification. We also need to listen for
-    // the tab close command since that means the load will never commit.  Note
-    // that in this case we don't need to close any previous infobar for this
-    // tab since this navigation will close it.
-    registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
-                   navigation_controller_source);
-    registrar_.Add(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
-                   tab_contents_source);
+    // Non-search navigation on a tab without one of our infobars.  This is
+    // irrelevant to us.
   }
-
-  infobar_map_[infobar_helper] = (*infobar_creator_)(infobar_helper, search_url,
-                                                     this, fetched_google_url_);
 }
 
 void GoogleURLTracker::OnNavigationCommittedOrTabClosed(
-    const content::NotificationSource& navigation_controller_source,
-    const content::NotificationSource& tab_contents_source,
     const InfoBarTabHelper* infobar_helper,
-    bool navigated) {
-  registrar_.Remove(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
-                    navigation_controller_source);
-  registrar_.Remove(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
-                    tab_contents_source);
-
+    const GURL& search_url) {
   InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
   DCHECK(i != infobar_map_.end());
-  DCHECK(need_to_prompt_);
-  if (navigated)
-    i->second->Show();
-  else
-    i->second->Close(false);  // Close manually since it's not added to a tab.
+  const MapEntry& map_entry = i->second;
+
+  if (!search_url.is_valid()) {
+    // Tab closed, or we somehow tried to navigate to an invalid URL (?!).
+    // InfoBarClosed() will take care of unregistering the notifications for
+    // this tab.
+    map_entry.infobar->Close(false);
+    return;
+  }
+
+  // We're getting called because of a commit notification, so pass true for
+  // |must_be_listening_for_commit|.
+  UnregisterForEntrySpecificNotifications(map_entry, true);
+  map_entry.infobar->Show(search_url);
 }
 
 void GoogleURLTracker::CloseAllInfoBars(bool redo_searches) {
   // Close all infobars, whether they've been added to tabs or not.
   while (!infobar_map_.empty())
-    infobar_map_.begin()->second->Close(redo_searches);
+    infobar_map_.begin()->second.infobar->Close(redo_searches);
+}
 
-  // Any registered listeners for NAV_ENTRY_COMMITTED and TAB_CLOSED are now
-  // irrelevant as the associated infobars are gone.
-  registrar_.RemoveAll();
+void GoogleURLTracker::UnregisterForEntrySpecificNotifications(
+    const MapEntry& map_entry,
+    bool must_be_listening_for_commit) {
+  // For tabs with non-showing infobars, we should always be listening for both
+  // these notifications.  For tabs with showing infobars, we may be listening
+  // for NOTIFICATION_NAV_ENTRY_COMMITTED if the user has performed a new search
+  // on this tab.
+  if (registrar_.IsRegistered(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+                              map_entry.navigation_controller_source)) {
+    registrar_.Remove(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+                      map_entry.navigation_controller_source);
+  } else {
+    DCHECK(!must_be_listening_for_commit);
+    DCHECK(map_entry.infobar->showing());
+  }
+  const bool registered_for_tab_contents_destroyed =
+      registrar_.IsRegistered(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
+                              map_entry.tab_contents_source);
+  DCHECK_NE(registered_for_tab_contents_destroyed,
+            map_entry.infobar->showing());
+  if (registered_for_tab_contents_destroyed) {
+    registrar_.Remove(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
+                      map_entry.tab_contents_source);
+  }
 }