Fix a number of problems with the GoogleURLTracker

chrome/browser/google/google_url_tracker.cc

Index: chrome/browser/google/google_url_tracker.cc
===================================================================
--- chrome/browser/google/google_url_tracker.cc	(revision 65992)
+++ chrome/browser/google/google_url_tracker.cc	(working copy)
@@ -27,13 +27,13 @@
 
 namespace {
 
-InfoBarDelegate* CreateInfobar(TabContents* tab_contents,
-                               GoogleURLTracker* google_url_tracker,
-                               const GURL& new_google_url) {
-  InfoBarDelegate* infobar = new GoogleURLTrackerInfoBarDelegate(tab_contents,
+GoogleURLTrackerInfoBarDelegate* CreateInfobar(
+    TabContents* tab_contents,
+    const GURL& search_url,
+    GoogleURLTracker* google_url_tracker,
+    const GURL& new_google_url) {
+  return new GoogleURLTrackerInfoBarDelegate(tab_contents, search_url,
       google_url_tracker, new_google_url);
-  tab_contents->AddInfoBar(infobar);
-  return infobar;
 }
 
 }  // namespace
@@ -42,36 +42,77 @@
 
 GoogleURLTrackerInfoBarDelegate::GoogleURLTrackerInfoBarDelegate(
     TabContents* tab_contents,
+    const GURL& search_url,
     GoogleURLTracker* google_url_tracker,
     const GURL& new_google_url)
     : ConfirmInfoBarDelegate(tab_contents),
+      tab_contents_(tab_contents),
+      search_url_(search_url),
       google_url_tracker_(google_url_tracker),
-      new_google_url_(new_google_url) {
+      new_google_url_(new_google_url),
+      showing_(false) {
 }
 
 bool GoogleURLTrackerInfoBarDelegate::Accept() {
   google_url_tracker_->AcceptGoogleURL(new_google_url_);
-  google_url_tracker_->RedoSearch();
-  return true;
+  return false;
 }
 
 bool GoogleURLTrackerInfoBarDelegate::Cancel() {
   google_url_tracker_->CancelGoogleURL(new_google_url_);
-  return true;
+  return false;
 }
 
 void GoogleURLTrackerInfoBarDelegate::InfoBarClosed() {
-  google_url_tracker_->InfoBarClosed();
+  if (google_url_tracker_)
+    google_url_tracker_->InfoBarClosed(this, tab_contents_);
   delete this;
 }
 
+void GoogleURLTrackerInfoBarDelegate::Show() {
+  tab_contents_->AddInfoBar(this);
+  showing_ = true;
+}
+
+void GoogleURLTrackerInfoBarDelegate::Close(bool redo_search) {
+  if (!showing_) {
+    // We haven't been added to a tab, so just delete ourselves.
+    InfoBarClosed();
+    return;
+  }
+
+  if (redo_search) {
+    // 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()) {
+      tab_contents_->OpenURL(new_search_url, GURL(), CURRENT_TAB,
+                             PageTransition::GENERATED);
+    }
+  }
+
+  // Synchronously remove ourselves from the URL tracker's list, because the
+  // RemoveInfoBar() call below may result in either a synchronous or an
+  // asynchronous call back to InfoBarClosed(), and it's easier to handle when
+  // we just guarantee the removal is synchronous.
+  google_url_tracker_->InfoBarClosed(this, tab_contents_);
+  google_url_tracker_ = NULL;
+
+  tab_contents_->RemoveInfoBar(this);
+}
+
 GoogleURLTrackerInfoBarDelegate::~GoogleURLTrackerInfoBarDelegate() {
 }
 
 string16 GoogleURLTrackerInfoBarDelegate::GetMessageText() const {
-  // TODO(ukai): change new_google_url to google_base_domain?
+  // Strip the "www." off the hostname since it's just noise.  This should
+  // always be present since we added it in OnURLFetchComplete().
+  std::string host(new_google_url_.host());
+  DCHECK(StartsWithASCII(host, "www.", true));
   return l10n_util::GetStringFUTF16(IDS_GOOGLE_URL_TRACKER_INFOBAR_MESSAGE,
-                                    UTF8ToUTF16(new_google_url_.spec()));
+                                    UTF8ToUTF16(host.substr(4)));
 }
 
 int GoogleURLTrackerInfoBarDelegate::GetButtons() const {
@@ -104,11 +145,11 @@
       already_fetched_(false),
       need_to_fetch_(false),
       request_context_available_(!!Profile::GetDefaultRequestContext()),
-      need_to_prompt_(false),
-      controller_(NULL),
-      infobar_(NULL) {
-  registrar_.Add(this, NotificationType::DEFAULT_REQUEST_CONTEXT_AVAILABLE,
-                 NotificationService::AllSources());
+      need_to_prompt_(false) {
+  if (!request_context_available_) {
+    registrar_.Add(this, NotificationType::DEFAULT_REQUEST_CONTEXT_AVAILABLE,
+                   NotificationService::AllSources());
+  }
 
   net::NetworkChangeNotifier::AddObserver(this);
 
@@ -127,6 +168,9 @@
 GoogleURLTracker::~GoogleURLTracker() {
   runnable_method_factory_.RevokeAll();
   net::NetworkChangeNotifier::RemoveObserver(this);
+  // We should only reach here after any tabs and their infobars have been torn
+  // down.
+  DCHECK(infobars_.empty());
 }
 
 // static
@@ -241,7 +285,6 @@
   GURL last_prompted_url(
       g_browser_process->local_state()->GetString(
           prefs::kLastPromptedGoogleURL));
-  need_to_prompt_ = false;
 
   if (last_prompted_url.is_empty()) {
     // On the very first run of Chrome, when we've never looked up the URL at
@@ -263,6 +306,10 @@
   }
 
   need_to_prompt_ = true;
+
+  // Any open infobars are pointing at the wrong Google URL.  (This can happen
+  // if an infobar has been sitting open and then our IP address changes.)
+  CloseAllInfobars(false);
 }
 
 void GoogleURLTracker::AcceptGoogleURL(const GURL& new_google_url) {
@@ -275,32 +322,23 @@
                                          NotificationService::AllSources(),
                                          NotificationService::NoDetails());
   need_to_prompt_ = false;
+  CloseAllInfobars(true);
 }
 
 void GoogleURLTracker::CancelGoogleURL(const GURL& new_google_url) {
   g_browser_process->local_state()->SetString(prefs::kLastPromptedGoogleURL,
                                               new_google_url.spec());
+  need_to_prompt_ = false;
+  CloseAllInfobars(false);
 }
 
-void GoogleURLTracker::InfoBarClosed() {
-  registrar_.RemoveAll();
-  controller_ = NULL;
-  infobar_ = NULL;
-  search_url_ = GURL();
+void GoogleURLTracker::InfoBarClosed(GoogleURLTrackerInfoBarDelegate* infobar,
+                                     TabContents* tab_contents) {
+  Infobars::iterator i(infobars_.find(tab_contents));
+  if ((i != infobars_.end()) && (i->second == infobar))
+    infobars_.erase(i);
 }
 
-void GoogleURLTracker::RedoSearch() {
-  //  Re-do the user's search on the new domain.
-  DCHECK(controller_);
-  url_canon::Replacements<char> replacements;
-  replacements.SetHost(google_url_.host().data(),
-                       url_parse::Component(0, google_url_.host().length()));
-  GURL new_search_url(search_url_.ReplaceComponents(replacements));
-  if (new_search_url.is_valid())
-    controller_->tab_contents()->OpenURL(new_search_url, GURL(), CURRENT_TAB,
-                                         PageTransition::GENERATED);
-}
-
 void GoogleURLTracker::Observe(NotificationType type,
                                const NotificationSource& source,
                                const NotificationDetails& details) {
@@ -313,13 +351,17 @@
       StartFetchIfDesirable();
       break;
 
-    case NotificationType::NAV_ENTRY_PENDING:
-      OnNavigationPending(source, controller_->pending_entry()->url());
+    case NotificationType::NAV_ENTRY_PENDING: {
+      NavigationController* controller =
+          Source<NavigationController>(source).ptr();
+      OnNavigationPending(source, controller->tab_contents(),
+                          controller->pending_entry()->url());
       break;
+    }
 
     case NotificationType::NAV_ENTRY_COMMITTED:
     case NotificationType::TAB_CLOSED:
-      OnNavigationCommittedOrTabClosed(
+      OnNavigationCommittedOrTabClosed(source,
           Source<NavigationController>(source).ptr()->tab_contents(),
           type.value);
       break;
@@ -335,7 +377,7 @@
 }
 
 void GoogleURLTracker::SearchCommitted() {
-  if (registrar_.IsEmpty() && (need_to_prompt_ || fetcher_.get())) {
+  if (need_to_prompt_) {
     // This notification will fire a bit later in the same call chain we're
     // currently in.
     registrar_.Add(this, NotificationType::NAV_ENTRY_PENDING,
@@ -344,37 +386,57 @@
 }
 
 void GoogleURLTracker::OnNavigationPending(const NotificationSource& source,
+                                           TabContents* tab_contents,
                                            const GURL& pending_url) {
-  controller_ = Source<NavigationController>(source).ptr();
-  search_url_ = pending_url;
   registrar_.Remove(this, NotificationType::NAV_ENTRY_PENDING,
                     NotificationService::AllSources());
-  // Start listening for the commit notification. We also need to listen for the
-  // tab close command since that means the load will never commit.
-  registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
-                 Source<NavigationController>(controller_));
-  registrar_.Add(this, NotificationType::TAB_CLOSED,
-                 Source<NavigationController>(controller_));
+
+  if (registrar_.IsRegistered(this, NotificationType::TAB_CLOSED, 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).
+    Infobars::iterator i(infobars_.find(tab_contents));
+    DCHECK(i != infobars_.end());
+    i->second->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
+    // TabContents since this navigation will close it.
+    registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, source);
+    registrar_.Add(this, NotificationType::TAB_CLOSED, source);
+  }
+
+  infobars_[tab_contents] =
+      (*infobar_creator_)(tab_contents, pending_url, this, fetched_google_url_);
 }
 
 void GoogleURLTracker::OnNavigationCommittedOrTabClosed(
+    const NotificationSource& source,
     TabContents* tab_contents,
     NotificationType::Type type) {
-  registrar_.RemoveAll();
+  registrar_.Remove(this, NotificationType::NAV_ENTRY_COMMITTED, source);
+  registrar_.Remove(this, NotificationType::TAB_CLOSED, source);
 
-  if (type == NotificationType::NAV_ENTRY_COMMITTED) {
-    ShowGoogleURLInfoBarIfNecessary(tab_contents);
-  } else {
-    controller_ = NULL;
-    infobar_ = NULL;
-  }
+  Infobars::iterator i(infobars_.find(tab_contents));
+  DCHECK(i != infobars_.end());
+  DCHECK(need_to_prompt_);

[Ilya Sherman, 2010/11/16 00:17:47]

Why does |need_to_prompt_| have to be true?

Suppose I start a search in one tab, and for whatever reason that search doesn't
commit.  Meanwhile, I open a second tab, perform a search, and accept the new
search URL.  Then I close both tabs.

It seems like |need_to_prompt_| would be false in that case.  Is this case not
reachable for some reason I'm overlooking?

[Peter Kasting, 2010/11/16 00:38:23]

Talked over this in person to ensure both of us that |need_to_prompt_| is indeed
true in this case, and all others.

+  if (type == NotificationType::NAV_ENTRY_COMMITTED)
+    i->second->Show();
+  else
+    i->second->Close(false);  // Close manually since it's not added to a tab.
 }
 
-void GoogleURLTracker::ShowGoogleURLInfoBarIfNecessary(
-    TabContents* tab_contents) {
-  if (!need_to_prompt_)
-    return;
-  DCHECK(!fetched_google_url_.is_empty());
+void GoogleURLTracker::CloseAllInfobars(bool redo_searches) {
+  // Close all infobars, whether they've been added to tabs or not.
+  while (!infobars_.empty())
+    infobars_.begin()->second->Close(redo_searches);
 
-  infobar_ = (*infobar_creator_)(tab_contents, this, fetched_google_url_);
+  // Any registered listeners for NAV_ENTRY_COMMITTED and TAB_CLOSED are now
+  // irrelevant as the associated infobars are gone.
+  registrar_.RemoveAll();
+  // We can't have had a listener for DEFAULT_REQUEST_CONTEXT_AVAILABLE, or we
+  // could never have triggered the search domain check and thus couldn't have
+  // reached here.
+  DCHECK(request_context_available_);
 }