Fix a number of problems with the GoogleURLTracker

chrome/browser/google/google_url_tracker.cc

 // Copyright (c) 2010 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 "chrome/browser/google/google_url_tracker.h"
 
 #include <vector>
 
 #include "app/l10n_util.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/net/url_fetcher_protect.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/pref_names.h"
 #include "grit/generated_resources.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request_status.h"
 
 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
 
 // GoogleURLTrackerInfoBarDelegate --------------------------------------------
 
 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 {
   return BUTTON_OK | BUTTON_CANCEL;
 }
 
 string16 GoogleURLTrackerInfoBarDelegate::GetButtonLabel(
     InfoBarButton button) const {
   return l10n_util::GetStringUTF16((button == BUTTON_OK) ?
                                    IDS_CONFIRM_MESSAGEBOX_YES_BUTTON_LABEL :
@@ skipping 12 matching lines @@
     : infobar_creator_(&CreateInfobar),
       google_url_(g_browser_process->local_state()->GetString(
           prefs::kLastKnownGoogleURL)),
       ALLOW_THIS_IN_INITIALIZER_LIST(runnable_method_factory_(this)),
       fetcher_id_(0),
       queue_wakeup_task_(true),
       in_startup_sleep_(true),
       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);
 
   // Configure to max_retries at most kMaxRetries times for 5xx errors.
   URLFetcherProtectEntry* protect =
       URLFetcherProtectManager::GetInstance()->Register(
           GURL(kSearchDomainCheckURL).host());
   static const int kMaxRetries = 5;
   protect->SetMaxRetries(kMaxRetries);
 
   MessageLoop::current()->PostTask(FROM_HERE,
                                    runnable_method_factory_.NewRunnableMethod(
                                    &GoogleURLTracker::QueueWakeupTask));
 }
 
 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
 GURL GoogleURLTracker::GoogleURL() {
   const GoogleURLTracker* const tracker =
       g_browser_process->google_url_tracker();
   return tracker ? tracker->google_url_ : GURL(kDefaultGoogleHomepage);
 }
 
 // static
@@ skipping 94 matching lines @@
   std::string url_str;
   TrimWhitespace(data, TRIM_ALL, &url_str);
 
   if (!StartsWithASCII(url_str, ".google.", false))
     return;
 
   fetched_google_url_ = GURL("http://www" + url_str);
   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
     // all, we should just silently switch over to whatever we get immediately.
     AcceptGoogleURL(fetched_google_url_);
     return;
   }
 
   // If the URL hasn't changed, then whether |need_to_prompt_| is true or false,
   // nothing has changed, so just bail.
   if (fetched_google_url_ == last_prompted_url)
     return;
 
   if (fetched_google_url_ == google_url_) {
     // The user came back to their original location after having temporarily
     // moved.  Reset the prompted URL so we'll prompt again if they move again.
     CancelGoogleURL(fetched_google_url_);
     return;
   }
 
   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) {
   google_url_ = new_google_url;
   g_browser_process->local_state()->SetString(prefs::kLastKnownGoogleURL,
                                               google_url_.spec());
   g_browser_process->local_state()->SetString(prefs::kLastPromptedGoogleURL,
                                               google_url_.spec());
   NotificationService::current()->Notify(NotificationType::GOOGLE_URL_UPDATED,
                                          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::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::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::Observe(NotificationType type,
                                const NotificationSource& source,
                                const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::DEFAULT_REQUEST_CONTEXT_AVAILABLE:
       registrar_.Remove(this,
                         NotificationType::DEFAULT_REQUEST_CONTEXT_AVAILABLE,
                         NotificationService::AllSources());
       request_context_available_ = true;
       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;
 
     default:
       NOTREACHED() << "Unknown notification received:" << type.value;
   }
 }
 
 void GoogleURLTracker::OnIPAddressChanged() {
   already_fetched_ = false;
   StartFetchIfDesirable();
 }
 
 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,
                    NotificationService::AllSources());
   }
 }
 
 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_);
 }