Revert of Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object....

Revert of Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object. (https://codereview.chromium.org/283413002/)

Reason for revert:
Introduced flake on Mac: crbug.com/376550

Original issue's description:
> Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object.
> 
> The goal of this CL is to eliminate the dependence of
> GoogleURLTracker(MapEntry) on NavigationController. To accomplish this goal,
> GoogleURLTrackerNavigationHelper is turned into a conceptually per-tab
> interface. GoogleURLTracker::OnNavigationPending() now takes in the
> GoogleURLTrackerNavigationHelper with which the navigation is associated rather
> than the associated NavigationController; GoogleURLTracker performs per-tab
> actions by calling the navigation helper associated with the tab.
> 
> A followup CL will turn GoogleURLTrackerNavigationHelper(Impl) into the
> GoogleURLTrackerDriver interface and ContentURLTrackerDriver implementation.
> 
> BUG=373230
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272094

TBR=pkasting@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=373230

Review URL: https://codereview.chromium.org/294193005

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

chrome/browser/google/chrome_google_url_tracker_client.cc

@@ -6,7 +6,6 @@
 
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_url_tracker.h"
-#include "chrome/browser/google/google_url_tracker_navigation_helper_impl.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
@@ -49,13 +48,12 @@ void ChromeGoogleURLTrackerClient::Observe(
       content::Source<content::NavigationController>(source).ptr();
   InfoBarService* infobar_service =
       InfoBarService::FromWebContents(controller->GetWebContents());
-  // Because we're listening to all sources, there may be no InfoBarService for
-  // some notifications, e.g. navigations in bubbles/balloons etc.
+  // Because we're listening to all sources, there may be no
+  // InfoBarService for some notifications, e.g. navigations in
+  // bubbles/balloons etc.
   if (infobar_service) {
     google_url_tracker()->OnNavigationPending(
-        scoped_ptr<GoogleURLTrackerNavigationHelper>(
-            new GoogleURLTrackerNavigationHelperImpl(
-                controller->GetWebContents(), google_url_tracker())),
+        controller,
         infobar_service,
         controller->GetPendingEntry()->GetUniqueID());
   }

chrome/browser/google/google_url_tracker.cc

@@ -33,11 +33,14 @@ const char GoogleURLTracker::kDefaultGoogleHomepage[] =
 const char GoogleURLTracker::kSearchDomainCheckURL[] =
     "https://www.google.com/searchdomaincheck?format=url&type=chrome";
 
-GoogleURLTracker::GoogleURLTracker(Profile* profile,
-                                   scoped_ptr<GoogleURLTrackerClient> client,
-                                   Mode mode)
+GoogleURLTracker::GoogleURLTracker(
+    Profile* profile,
+    scoped_ptr<GoogleURLTrackerClient> client,
+    scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
+    Mode mode)
     : profile_(profile),
       client_(client.Pass()),
+      nav_helper_(nav_helper.Pass()),
       infobar_creator_(base::Bind(&GoogleURLTrackerInfoBarDelegate::Create)),
       google_url_(mode == UNIT_TEST_MODE ?
           kDefaultGoogleHomepage :
@@ -51,6 +54,7 @@ GoogleURLTracker::GoogleURLTracker(Profile* profile,
       weak_ptr_factory_(this) {
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
   client_->set_google_url_tracker(this);
+  nav_helper_->SetGoogleURLTracker(this);
 
   // Because this function can be called during startup, when kicking off a URL
   // fetch can eat up 20 ms of time, we delay five seconds, which is hopefully
@@ -201,6 +205,7 @@ void GoogleURLTracker::OnIPAddressChanged() {
 
 void GoogleURLTracker::Shutdown() {
   client_.reset();
+  nav_helper_.reset();
   fetcher_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
   net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
@@ -214,7 +219,7 @@ void GoogleURLTracker::DeleteMapEntryForService(
   DCHECK(i != entry_map_.end());
   GoogleURLTrackerMapEntry* map_entry = i->second;
 
-  UnregisterForEntrySpecificNotifications(map_entry, false);
+  UnregisterForEntrySpecificNotifications(*map_entry, false);
   entry_map_.erase(i);
   delete map_entry;
 }
@@ -276,41 +281,39 @@ void GoogleURLTracker::SearchCommitted() {
 }
 
 void GoogleURLTracker::OnNavigationPending(
-    scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
+    content::NavigationController* navigation_controller,
     InfoBarService* infobar_service,
     int pending_id) {
-  GoogleURLTrackerMapEntry* map_entry = NULL;
-
   EntryMap::iterator i(entry_map_.find(infobar_service));
-  if (i != entry_map_.end())
-    map_entry = i->second;
 
   if (search_committed_) {
     search_committed_ = false;
-    if (!map_entry) {
+    // 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 (!nav_helper_->IsListeningForNavigationCommit(navigation_controller)) {
+      nav_helper_->SetListeningForNavigationCommit(navigation_controller,
+                                                   true);
+    }
+    if (i == entry_map_.end()) {
       // This is a search on a tab that doesn't have one of our infobars, so
       // prepare to add one.  Note that we only listen for the tab's destruction
       // on this path; if there was already a map entry, then either it doesn't
       // yet have an infobar and we're already registered for this, or it has an
       // infobar and the infobar's owner will handle tearing it down when the
       // tab is destroyed.
-      map_entry = new GoogleURLTrackerMapEntry(
-          this, infobar_service, nav_helper.Pass());
-      map_entry->navigation_helper()->SetListeningForTabDestruction(true);
-      entry_map_.insert(std::make_pair(infobar_service, map_entry));
-    } else if (map_entry->infobar_delegate()) {
+      nav_helper_->SetListeningForTabDestruction(navigation_controller, true);
+      entry_map_.insert(std::make_pair(
+          infobar_service,
+          new GoogleURLTrackerMapEntry(this, infobar_service,
+                                       navigation_controller)));
+    } else if (i->second->has_infobar_delegate()) {
       // This is a new search on a tab where we already have an infobar.
-      map_entry->infobar_delegate()->set_pending_id(pending_id);
+      i->second->infobar_delegate()->set_pending_id(pending_id);
     }
-
-    // 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 (!map_entry->navigation_helper()->IsListeningForNavigationCommit())
-      map_entry->navigation_helper()->SetListeningForNavigationCommit(true);
-  } else if (map_entry) {
-    if (map_entry->has_infobar_delegate()) {
+  } else if (i != entry_map_.end()){
+    if (i->second->has_infobar_delegate()) {
       // This is a non-search navigation on a tab with an 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
@@ -320,13 +323,13 @@ void GoogleURLTracker::OnNavigationPending(
       // 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(map_entry, false);
-      map_entry->infobar_delegate()->set_pending_id(0);
+      UnregisterForEntrySpecificNotifications(*i->second, false);
+      i->second->infobar_delegate()->set_pending_id(0);
     } else {
       // Non-search navigation on a tab with an entry that has not yet created
       // an infobar.  This means the original search won't commit, so delete the
       // entry.
-      map_entry->Close(false);
+      i->second->Close(false);
     }
   } else {
     // Non-search navigation on a tab without an infobars.  This is irrelevant
@@ -341,7 +344,7 @@ void GoogleURLTracker::OnNavigationCommitted(InfoBarService* infobar_service,
   GoogleURLTrackerMapEntry* map_entry = i->second;
   DCHECK(search_url.is_valid());
 
-  UnregisterForEntrySpecificNotifications(map_entry, true);
+  UnregisterForEntrySpecificNotifications(*map_entry, true);
   if (map_entry->has_infobar_delegate()) {
     map_entry->infobar_delegate()->Update(search_url);
   } else {
@@ -357,7 +360,7 @@ void GoogleURLTracker::OnNavigationCommitted(InfoBarService* infobar_service,
 }
 
 void GoogleURLTracker::OnTabClosed(
-    GoogleURLTrackerNavigationHelper* nav_helper) {
+    content::NavigationController* navigation_controller) {
   // Because InfoBarService tears itself down on tab destruction, it's possible
   // to get a non-NULL InfoBarService pointer here, depending on which order
   // notifications fired in.  Likewise, the pointer in |entry_map_| (and in its
@@ -367,7 +370,7 @@ void GoogleURLTracker::OnTabClosed(
   // function doesn't need to do even that, but others in the call chain from
   // here might (and have comments pointing back here).
   for (EntryMap::iterator i(entry_map_.begin()); i != entry_map_.end(); ++i) {
-    if (i->second->navigation_helper() == nav_helper) {
+    if (i->second->navigation_controller() == navigation_controller) {
       i->second->Close(false);
       return;
     }
@@ -387,22 +390,26 @@ void GoogleURLTracker::CloseAllEntries(bool redo_searches) {
 }
 
 void GoogleURLTracker::UnregisterForEntrySpecificNotifications(
-    GoogleURLTrackerMapEntry* map_entry,
+    const GoogleURLTrackerMapEntry& map_entry,
     bool must_be_listening_for_commit) {
   // For tabs with map entries but no infobars, we should always be listening
   // for both these notifications.  For tabs with infobars, we may be listening
   // for navigation commits if the user has performed a new search on this tab.
-  if (map_entry->navigation_helper()->IsListeningForNavigationCommit()) {
-    map_entry->navigation_helper()->SetListeningForNavigationCommit(false);
+  if (nav_helper_->IsListeningForNavigationCommit(
+          map_entry.navigation_controller())) {
+    nav_helper_->SetListeningForNavigationCommit(
+        map_entry.navigation_controller(), false);
   } else {
     DCHECK(!must_be_listening_for_commit);
-    DCHECK(map_entry->has_infobar_delegate());
+    DCHECK(map_entry.has_infobar_delegate());
   }
   const bool registered_for_tab_destruction =
-      map_entry->navigation_helper()->IsListeningForTabDestruction();
-  DCHECK_NE(registered_for_tab_destruction, map_entry->has_infobar_delegate());
+      nav_helper_->IsListeningForTabDestruction(
+          map_entry.navigation_controller());
+  DCHECK_NE(registered_for_tab_destruction, map_entry.has_infobar_delegate());
   if (registered_for_tab_destruction) {
-    map_entry->navigation_helper()->SetListeningForTabDestruction(false);
+    nav_helper_->SetListeningForTabDestruction(
+        map_entry.navigation_controller(), false);
   }
 
   // Our global listeners for these other notifications should be in place iff
@@ -412,7 +419,8 @@ void GoogleURLTracker::UnregisterForEntrySpecificNotifications(
   // See the various cases inside OnNavigationPending().
   for (EntryMap::const_iterator i(entry_map_.begin()); i != entry_map_.end();
        ++i) {
-    if (i->second->navigation_helper()->IsListeningForNavigationCommit()) {
+    if (nav_helper_->IsListeningForNavigationCommit(
+            i->second->navigation_controller())) {
       DCHECK(client_->IsListeningForNavigationStart());
       return;
     }

chrome/browser/google/google_url_tracker.h

@@ -26,6 +26,10 @@ class GoogleURLTrackerNavigationHelper;
 class PrefService;
 class Profile;
 
+namespace content {
+class NavigationController;
+}
+
 namespace infobars {
 class InfoBar;
 }
@@ -68,6 +72,7 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // GoogleURLTrackerFactory::GetForProfile().
   GoogleURLTracker(Profile* profile,
                    scoped_ptr<GoogleURLTrackerClient> client,
+                   scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
                    Mode mode);
 
   virtual ~GoogleURLTracker();
@@ -108,14 +113,14 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
 
   // Called by the client after SearchCommitted() registers listeners, to
   // indicate that we've received the "load now pending" notification.
-  // |nav_helper| is the GoogleURLTrackerNavigationHelper associated with this
-  // navigation; |infobar_service| is the InfoBarService of the associated tab;
-  // and |pending_id| is the unique ID of the newly pending NavigationEntry.
-  // If there is already a visible GoogleURLTracker infobar for this tab, this
+  // |navigation_controller| is the NavigationController for this load;
+  // |infobar_service| is the InfoBarService of the associated tab; and
+  // |pending_id| is the unique ID of the newly pending NavigationEntry.  If
+  // there is already a visible GoogleURLTracker infobar for this tab, this
   // function resets its associated pending entry ID to the new ID.  Otherwise
   // this function creates a map entry for the associated tab.
   virtual void OnNavigationPending(
-      scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
+      content::NavigationController* navigation_controller,
       InfoBarService* infobar_service,
       int pending_id);
 
@@ -126,7 +131,8 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
                                      const GURL& search_url);
 
   // Called by the navigation observer when a tab closes.
-  virtual void OnTabClosed(GoogleURLTrackerNavigationHelper* nav_helper);
+  virtual void OnTabClosed(
+      content::NavigationController* navigation_controller);
 
   scoped_ptr<Subscription> RegisterCallback(
       const OnGoogleURLUpdatedCallback& cb);
@@ -168,14 +174,14 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // Google TLD.
   void CloseAllEntries(bool redo_searches);
 
-  // Unregisters any listeners for the navigation helper in |map_entry|.
+  // Unregisters any listeners for the navigation controller in |map_entry|.
   // This sanity-DCHECKs that these are registered (or not) in the specific
   // cases we expect.  (|must_be_listening_for_commit| is used purely for this
   // sanity-checking.)  This also unregisters the global navigation pending
   // listener if there are no remaining listeners for navigation commits, as we
   // no longer need them until another search is committed.
   void UnregisterForEntrySpecificNotifications(
-      GoogleURLTrackerMapEntry* map_entry,
+      const GoogleURLTrackerMapEntry& map_entry,
       bool must_be_listening_for_commit);
 
   void NotifyGoogleURLUpdated(GURL old_url, GURL new_url);
@@ -186,6 +192,8 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
 
   scoped_ptr<GoogleURLTrackerClient> client_;
 
+  scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper_;
+
   // Creates an infobar and adds it to the provided InfoBarService.  Returns the
   // infobar on success or NULL on failure.  The caller does not own the
   // returned object, the InfoBarService does.

chrome/browser/google/google_url_tracker_factory.cc

@@ -7,6 +7,7 @@
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/google/chrome_google_url_tracker_client.h"
 #include "chrome/browser/google/google_url_tracker.h"
+#include "chrome/browser/google/google_url_tracker_navigation_helper_impl.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
@@ -37,8 +38,11 @@ GoogleURLTrackerFactory::~GoogleURLTrackerFactory() {
 KeyedService* GoogleURLTrackerFactory::BuildServiceInstanceFor(
     content::BrowserContext* profile) const {
   scoped_ptr<GoogleURLTrackerClient> client(new ChromeGoogleURLTrackerClient());
+  scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper(
+      new GoogleURLTrackerNavigationHelperImpl());
   return new GoogleURLTracker(static_cast<Profile*>(profile),
                               client.Pass(),
+                              nav_helper.Pass(),
                               GoogleURLTracker::NORMAL_MODE);
 }
 

chrome/browser/google/google_url_tracker_map_entry.cc

@@ -15,11 +15,11 @@
 GoogleURLTrackerMapEntry::GoogleURLTrackerMapEntry(
     GoogleURLTracker* google_url_tracker,
     InfoBarService* infobar_service,
-    scoped_ptr<GoogleURLTrackerNavigationHelper> navigation_helper)
+    const content::NavigationController* navigation_controller)
     : google_url_tracker_(google_url_tracker),
       infobar_service_(infobar_service),
       infobar_delegate_(NULL),
-      navigation_helper_(navigation_helper.Pass()) {
+      navigation_controller_(navigation_controller) {
 }
 
 GoogleURLTrackerMapEntry::~GoogleURLTrackerMapEntry() {

chrome/browser/google/google_url_tracker_map_entry.h

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_MAP_ENTRY_H_
 #define CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_MAP_ENTRY_H_
 
-#include "base/memory/scoped_ptr.h"
-#include "chrome/browser/google/google_url_tracker_navigation_helper.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
@@ -14,12 +12,16 @@ class GoogleURLTracker;
 class GoogleURLTrackerInfoBarDelegate;
 class InfoBarService;
 
+namespace content {
+class NavigationController;
+}
+
 class GoogleURLTrackerMapEntry : public content::NotificationObserver {
  public:
   GoogleURLTrackerMapEntry(
       GoogleURLTracker* google_url_tracker,
       InfoBarService* infobar_service,
-      scoped_ptr<GoogleURLTrackerNavigationHelper> navigation_helper);
+      const content::NavigationController* navigation_controller);
   virtual ~GoogleURLTrackerMapEntry();
 
   bool has_infobar_delegate() const { return !!infobar_delegate_; }
@@ -28,8 +30,8 @@ class GoogleURLTrackerMapEntry : public content::NotificationObserver {
   }
   void SetInfoBarDelegate(GoogleURLTrackerInfoBarDelegate* infobar_delegate);
 
-  GoogleURLTrackerNavigationHelper* navigation_helper() {
-    return navigation_helper_.get();
+  const content::NavigationController* navigation_controller() const {
+    return navigation_controller_;
   }
 
   void Close(bool redo_search);
@@ -46,7 +48,7 @@ class GoogleURLTrackerMapEntry : public content::NotificationObserver {
   GoogleURLTracker* const google_url_tracker_;
   const InfoBarService* const infobar_service_;
   GoogleURLTrackerInfoBarDelegate* infobar_delegate_;
-  scoped_ptr<GoogleURLTrackerNavigationHelper> navigation_helper_;
+  const content::NavigationController* const navigation_controller_;
 
   DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerMapEntry);
 };

chrome/browser/google/google_url_tracker_navigation_helper.cc

-// Copyright 2014 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_navigation_helper.h"
-
-GoogleURLTrackerNavigationHelper::GoogleURLTrackerNavigationHelper(
-    GoogleURLTracker* google_url_tracker)
-    : google_url_tracker_(google_url_tracker) {
-}
-
-GoogleURLTrackerNavigationHelper::~GoogleURLTrackerNavigationHelper() {
-}

chrome/browser/google/google_url_tracker_navigation_helper.h

@@ -5,42 +5,46 @@
 #ifndef CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_H_
 #define CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_H_
 
-#include "base/macros.h"
-
 class GoogleURLTracker;
+class InfoBarService;
+class Profile;
+
+namespace content {
+class NavigationController;
+}
 
 // A helper class for GoogleURLTracker that abstracts the details of listening
 // for various navigation events.
 class GoogleURLTrackerNavigationHelper {
  public:
-  explicit GoogleURLTrackerNavigationHelper(
-      GoogleURLTracker* google_url_tracker);
-  virtual ~GoogleURLTrackerNavigationHelper();
-
-  // Enables or disables listening for navigation commits.
-  // OnNavigationCommitted will be called for each navigation commit if
-  // listening is enabled.
-  virtual void SetListeningForNavigationCommit(bool listen) = 0;
+  virtual ~GoogleURLTrackerNavigationHelper() {}
 
-  // Returns whether or not this object is currently listening for navigation
-  // commits.
-  virtual bool IsListeningForNavigationCommit() = 0;
+  // Sets the GoogleURLTracker that is associated with this object.
+  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) = 0;
 
-  // Enables or disables listening for tab destruction. OnTabClosed will be
-  // called on tab destruction if listening is enabled.
-  virtual void SetListeningForTabDestruction(bool listen) = 0;
+  // Enables or disables listening for navigation commits for the given
+  // NavigationController. OnNavigationCommitted will be called for each
+  // navigation commit if listening is enabled.
+  virtual void SetListeningForNavigationCommit(
+      const content::NavigationController* nav_controller,
+      bool listen) = 0;
 
-  // Returns whether or not this object is currently listening for tab
-  // destruction.
-  virtual bool IsListeningForTabDestruction() = 0;
+  // Returns whether or not the observer is currently listening for navigation
+  // commits for the given NavigationController.
+  virtual bool IsListeningForNavigationCommit(
+      const content::NavigationController* nav_controller) = 0;
 
- protected:
-  GoogleURLTracker* google_url_tracker() { return google_url_tracker_; }
-
- private:
-  GoogleURLTracker* google_url_tracker_;
-
-  DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerNavigationHelper);
+  // Enables or disables listening for tab destruction for the given
+  // NavigationController. OnTabClosed will be called on tab destruction if
+  // listening is enabled.
+  virtual void SetListeningForTabDestruction(
+      const content::NavigationController* nav_controller,
+      bool listen) = 0;
+
+  // Returns whether or not the observer is currently listening for tab
+  // destruction for the given NavigationController.
+  virtual bool IsListeningForTabDestruction(
+      const content::NavigationController* nav_controller) = 0;
 };
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_H_

chrome/browser/google/google_url_tracker_navigation_helper_impl.cc

@@ -12,21 +12,25 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 
-GoogleURLTrackerNavigationHelperImpl::GoogleURLTrackerNavigationHelperImpl(
-    content::WebContents* web_contents,
-    GoogleURLTracker* tracker)
-    : GoogleURLTrackerNavigationHelper(tracker),
-      web_contents_(web_contents) {
+GoogleURLTrackerNavigationHelperImpl::
+    GoogleURLTrackerNavigationHelperImpl() : tracker_(NULL) {
 }
 
-GoogleURLTrackerNavigationHelperImpl::~GoogleURLTrackerNavigationHelperImpl() {
+GoogleURLTrackerNavigationHelperImpl::
+    ~GoogleURLTrackerNavigationHelperImpl() {
+}
+
+void GoogleURLTrackerNavigationHelperImpl::SetGoogleURLTracker(
+    GoogleURLTracker* tracker) {
+  DCHECK(tracker);
+  tracker_ = tracker;
 }
 
 void GoogleURLTrackerNavigationHelperImpl::SetListeningForNavigationCommit(
+    const content::NavigationController* nav_controller,
     bool listen) {
   content::NotificationSource navigation_controller_source =
-      content::Source<content::NavigationController>(
-          &web_contents_->GetController());
+      content::Source<content::NavigationController>(nav_controller);
   if (listen) {
     registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                     navigation_controller_source);
@@ -36,35 +40,47 @@ void GoogleURLTrackerNavigationHelperImpl::SetListeningForNavigationCommit(
   }
 }
 
-bool GoogleURLTrackerNavigationHelperImpl::IsListeningForNavigationCommit() {
+bool GoogleURLTrackerNavigationHelperImpl::IsListeningForNavigationCommit(
+    const content::NavigationController* nav_controller) {
   content::NotificationSource navigation_controller_source =
-      content::Source<content::NavigationController>(
-          &web_contents_->GetController());
+      content::Source<content::NavigationController>(nav_controller);
   return registrar_.IsRegistered(
       this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
       navigation_controller_source);
 }
 
 void GoogleURLTrackerNavigationHelperImpl::SetListeningForTabDestruction(
+    const content::NavigationController* nav_controller,
     bool listen) {
-  content::NotificationSource web_contents_source =
-      content::Source<content::WebContents>(web_contents_);
+  content::NotificationSource navigation_controller_source =
+      content::Source<content::NavigationController>(nav_controller);
   if (listen) {
-    registrar_.Add(this,
-                   content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                   web_contents_source);
+    registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                   GetWebContentsSource(navigation_controller_source));
   } else {
-    registrar_.Remove(this,
-                      content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                      web_contents_source);
+    registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                      GetWebContentsSource(navigation_controller_source));
   }
 }
 
-bool GoogleURLTrackerNavigationHelperImpl::IsListeningForTabDestruction() {
+bool GoogleURLTrackerNavigationHelperImpl::IsListeningForTabDestruction(
+    const content::NavigationController* nav_controller) {
+  content::NotificationSource navigation_controller_source =
+      content::Source<content::NavigationController>(nav_controller);
   return registrar_.IsRegistered(
       this,
       content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-      content::Source<content::WebContents>(web_contents_));
+      GetWebContentsSource(navigation_controller_source));
+}
+
+content::NotificationSource
+    GoogleURLTrackerNavigationHelperImpl::GetWebContentsSource(
+        const content::NotificationSource& nav_controller_source) {
+  content::NavigationController* controller =
+      content::Source<content::NavigationController>(
+          nav_controller_source).ptr();
+  content::WebContents* web_contents = controller->GetWebContents();
+  return content::Source<content::WebContents>(web_contents);
 }
 
 void GoogleURLTrackerNavigationHelperImpl::Observe(
@@ -75,24 +91,23 @@ void GoogleURLTrackerNavigationHelperImpl::Observe(
     case content::NOTIFICATION_NAV_ENTRY_COMMITTED: {
       content::NavigationController* controller =
           content::Source<content::NavigationController>(source).ptr();
-      DCHECK_EQ(web_contents_, controller->GetWebContents());
-
       // Here we're only listening to notifications where we already know
       // there's an associated InfoBarService.
+      content::WebContents* web_contents = controller->GetWebContents();
       InfoBarService* infobar_service =
-          InfoBarService::FromWebContents(web_contents_);
+          InfoBarService::FromWebContents(web_contents);
       DCHECK(infobar_service);
       const GURL& search_url = controller->GetActiveEntry()->GetURL();
       if (!search_url.is_valid())  // Not clear if this can happen.
-        google_url_tracker()->OnTabClosed(this);
-      google_url_tracker()->OnNavigationCommitted(infobar_service, search_url);
+        tracker_->OnTabClosed(controller);
+      tracker_->OnNavigationCommitted(infobar_service, search_url);
       break;
     }
 
     case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
-      DCHECK_EQ(web_contents_,
-                content::Source<content::WebContents>(source).ptr());
-      google_url_tracker()->OnTabClosed(this);
+      content::WebContents* web_contents =
+          content::Source<content::WebContents>(source).ptr();
+      tracker_->OnTabClosed(&web_contents->GetController());
       break;
     }
 

chrome/browser/google/google_url_tracker_navigation_helper_impl.h

@@ -10,25 +10,25 @@
 #include "content/public/browser/notification_registrar.h"
 #include "url/gurl.h"
 
-namespace content {
-class WebContents;
-}
-
 class GoogleURLTrackerNavigationHelperImpl
     : public GoogleURLTrackerNavigationHelper,
       public content::NotificationObserver {
  public:
-  GoogleURLTrackerNavigationHelperImpl(content::WebContents* web_contents,
-                                       GoogleURLTracker* tracker);
+  explicit GoogleURLTrackerNavigationHelperImpl();
   virtual ~GoogleURLTrackerNavigationHelperImpl();
 
-  // GoogleURLTrackerNavigationHelper:
+  // GoogleURLTrackerNavigationHelper.
+  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
   virtual void SetListeningForNavigationCommit(
+      const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
-  virtual bool IsListeningForNavigationCommit() OVERRIDE;
+  virtual bool IsListeningForNavigationCommit(
+      const content::NavigationController* nav_controller) OVERRIDE;
   virtual void SetListeningForTabDestruction(
+      const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
-  virtual bool IsListeningForTabDestruction() OVERRIDE;
+  virtual bool IsListeningForTabDestruction(
+      const content::NavigationController* nav_controller) OVERRIDE;
 
  private:
   // content::NotificationObserver:
@@ -36,10 +36,13 @@ class GoogleURLTrackerNavigationHelperImpl
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  content::WebContents* web_contents_;
-  content::NotificationRegistrar registrar_;
+  // Returns a WebContents NavigationSource for the WebContents corresponding to
+  // the given NavigationController NotificationSource.
+  virtual content::NotificationSource GetWebContentsSource(
+      const content::NotificationSource& nav_controller_source);
 
-  DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerNavigationHelperImpl);
+  GoogleURLTracker* tracker_;
+  content::NotificationRegistrar registrar_;
 };
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_IMPL_H_

chrome/browser/google/google_url_tracker_unittest.cc

@@ -137,53 +137,73 @@ bool TestGoogleURLTrackerClient::IsListeningForNavigationStart() {
   return observe_nav_start_;
 }
 
-
 // TestGoogleURLTrackerNavigationHelper ---------------------------------------
 
 class TestGoogleURLTrackerNavigationHelper
     : public GoogleURLTrackerNavigationHelper {
  public:
-  explicit TestGoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
+  TestGoogleURLTrackerNavigationHelper();
   virtual ~TestGoogleURLTrackerNavigationHelper();
 
-  virtual void SetListeningForNavigationCommit(bool listen) OVERRIDE;
-  virtual bool IsListeningForNavigationCommit() OVERRIDE;
-  virtual void SetListeningForTabDestruction(bool listen) OVERRIDE;
-  virtual bool IsListeningForTabDestruction() OVERRIDE;
+  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
+  virtual void SetListeningForNavigationCommit(
+      const content::NavigationController* nav_controller,
+      bool listen) OVERRIDE;
+  virtual bool IsListeningForNavigationCommit(
+      const content::NavigationController* nav_controller) OVERRIDE;
+  virtual void SetListeningForTabDestruction(
+      const content::NavigationController* nav_controller,
+      bool listen) OVERRIDE;
+  virtual bool IsListeningForTabDestruction(
+      const content::NavigationController* nav_controller) OVERRIDE;
 
  private:
-  bool listening_for_nav_commit_;
-  bool listening_for_tab_destruction_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestGoogleURLTrackerNavigationHelper);
+  GoogleURLTracker* tracker_;
+  std::set<const content::NavigationController*>
+      nav_controller_commit_listeners_;
+  std::set<const content::NavigationController*>
+      nav_controller_tab_close_listeners_;
 };
 
-TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper(
-    GoogleURLTracker* tracker)
-    : GoogleURLTrackerNavigationHelper(tracker),
-      listening_for_nav_commit_(false),
-      listening_for_tab_destruction_(false) {
+TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper()
+    : tracker_(NULL) {
+}
+
+TestGoogleURLTrackerNavigationHelper::
+    ~TestGoogleURLTrackerNavigationHelper() {
 }
 
-TestGoogleURLTrackerNavigationHelper::~TestGoogleURLTrackerNavigationHelper() {
+void TestGoogleURLTrackerNavigationHelper::SetGoogleURLTracker(
+    GoogleURLTracker* tracker) {
+  tracker_ = tracker;
 }
 
 void TestGoogleURLTrackerNavigationHelper::SetListeningForNavigationCommit(
+    const content::NavigationController* nav_controller,
     bool listen) {
-  listening_for_nav_commit_ = listen;
+  if (listen)
+    nav_controller_commit_listeners_.insert(nav_controller);
+  else
+    nav_controller_commit_listeners_.erase(nav_controller);
 }
 
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationCommit() {
-  return listening_for_nav_commit_;
+bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationCommit(
+    const content::NavigationController* nav_controller) {
+  return nav_controller_commit_listeners_.count(nav_controller) > 0;
 }
 
 void TestGoogleURLTrackerNavigationHelper::SetListeningForTabDestruction(
+    const content::NavigationController* nav_controller,
     bool listen) {
-  listening_for_tab_destruction_ = listen;
+  if (listen)
+    nav_controller_tab_close_listeners_.insert(nav_controller);
+  else
+    nav_controller_tab_close_listeners_.erase(nav_controller);
 }
 
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction() {
-  return listening_for_tab_destruction_;
+bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction(
+    const content::NavigationController* nav_controller) {
+  return nav_controller_tab_close_listeners_.count(nav_controller) > 0;
 }
 
 }  // namespace
@@ -240,7 +260,6 @@ class GoogleURLTrackerTest : public testing::Test {
   void CloseTab(intptr_t unique_id);
   GoogleURLTrackerMapEntry* GetMapEntry(intptr_t unique_id);
   GoogleURLTrackerInfoBarDelegate* GetInfoBarDelegate(intptr_t unique_id);
-  GoogleURLTrackerNavigationHelper* GetNavigationHelper(intptr_t unique_id);
   void ExpectDefaultURLs() const;
   void ExpectListeningForCommit(intptr_t unique_id, bool listening);
   bool listener_notified() const { return listener_.notified(); }
@@ -263,6 +282,7 @@ class GoogleURLTrackerTest : public testing::Test {
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   net::TestURLFetcherFactory fetcher_factory_;
   GoogleURLTrackerClient* client_;
+  GoogleURLTrackerNavigationHelper* nav_helper_;
   TestingProfile profile_;
   scoped_ptr<GoogleURLTracker> google_url_tracker_;
   TestCallbackListener listener_;
@@ -301,12 +321,17 @@ GoogleURLTrackerTest::~GoogleURLTrackerTest() {
 
 void GoogleURLTrackerTest::SetUp() {
   network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
-  // Ownership is passed to google_url_tracker_, but a weak pointer is kept;
-  // this is safe since GoogleURLTracker keeps the client for its lifetime.
+  // Ownership is passed to google_url_tracker_, but weak pointers are kept;
+  // this is safe since GoogleURLTracker keeps these objects for its lifetime.
   client_ = new TestGoogleURLTrackerClient();
+  nav_helper_ = new TestGoogleURLTrackerNavigationHelper();
   scoped_ptr<GoogleURLTrackerClient> client(client_);
-  google_url_tracker_.reset(new GoogleURLTracker(
-      &profile_, client.Pass(), GoogleURLTracker::UNIT_TEST_MODE));
+  scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper(nav_helper_);
+  google_url_tracker_.reset(
+      new GoogleURLTracker(&profile_,
+                           client.Pass(),
+                           nav_helper.Pass(),
+                           GoogleURLTracker::UNIT_TEST_MODE));
   google_url_tracker_->infobar_creator_ = base::Bind(
       &GoogleURLTrackerTest::CreateTestInfoBar, base::Unretained(this));
 }
@@ -370,11 +395,8 @@ void GoogleURLTrackerTest::SetNavigationPending(intptr_t unique_id,
   unique_ids_seen_.insert(unique_id);
   if (client_->IsListeningForNavigationStart()) {
     google_url_tracker_->OnNavigationPending(
-        scoped_ptr<GoogleURLTrackerNavigationHelper>(
-            new TestGoogleURLTrackerNavigationHelper(
-                google_url_tracker_.get())),
-        reinterpret_cast<InfoBarService*>(unique_id),
-        unique_id);
+        reinterpret_cast<content::NavigationController*>(unique_id),
+        reinterpret_cast<InfoBarService*>(unique_id), unique_id);
   }
 }
 
@@ -400,8 +422,8 @@ void GoogleURLTrackerTest::CommitNonSearch(intptr_t unique_id) {
 void GoogleURLTrackerTest::CommitSearch(intptr_t unique_id,
                                         const GURL& search_url) {
   DCHECK(search_url.is_valid());
-  GoogleURLTrackerNavigationHelper* nav_helper = GetNavigationHelper(unique_id);
-  if (nav_helper && nav_helper->IsListeningForNavigationCommit()) {
+  if (nav_helper_->IsListeningForNavigationCommit(
+      reinterpret_cast<content::NavigationController*>(unique_id))) {
     google_url_tracker_->OnNavigationCommitted(
         reinterpret_cast<InfoBarService*>(unique_id), search_url);
   }
@@ -409,9 +431,10 @@ void GoogleURLTrackerTest::CommitSearch(intptr_t unique_id,
 
 void GoogleURLTrackerTest::CloseTab(intptr_t unique_id) {
   unique_ids_seen_.erase(unique_id);
-  GoogleURLTrackerNavigationHelper* nav_helper = GetNavigationHelper(unique_id);
-  if (nav_helper && nav_helper->IsListeningForTabDestruction()) {
-    google_url_tracker_->OnTabClosed(nav_helper);
+  content::NavigationController* nav_controller =
+      reinterpret_cast<content::NavigationController*>(unique_id);
+  if (nav_helper_->IsListeningForTabDestruction(nav_controller)) {
+    google_url_tracker_->OnTabClosed(nav_controller);
   } else {
     // Closing a tab with an infobar showing would close the infobar.
     GoogleURLTrackerInfoBarDelegate* delegate = GetInfoBarDelegate(unique_id);
@@ -434,12 +457,6 @@ GoogleURLTrackerInfoBarDelegate* GoogleURLTrackerTest::GetInfoBarDelegate(
   return map_entry ? map_entry->infobar_delegate() : NULL;
 }
 
-GoogleURLTrackerNavigationHelper* GoogleURLTrackerTest::GetNavigationHelper(
-    intptr_t unique_id) {
-  GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
-  return map_entry ? map_entry->navigation_helper() : NULL;
-}
-
 void GoogleURLTrackerTest::ExpectDefaultURLs() const {
   EXPECT_EQ(GURL(GoogleURLTracker::kDefaultGoogleHomepage), google_url());
   EXPECT_EQ(GURL(), fetched_google_url());
@@ -449,8 +466,8 @@ void GoogleURLTrackerTest::ExpectListeningForCommit(intptr_t unique_id,
                                                     bool listening) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (map_entry) {
-    EXPECT_EQ(listening,
-              map_entry->navigation_helper()->IsListeningForNavigationCommit());
+    EXPECT_EQ(listening, nav_helper_->IsListeningForNavigationCommit(
+        map_entry->navigation_controller()));
   } else {
     EXPECT_FALSE(listening);
   }

chrome/chrome_browser.gypi

@@ -696,7 +696,6 @@
         'browser/google/google_url_tracker_infobar_delegate.h',
         'browser/google/google_url_tracker_map_entry.cc',
         'browser/google/google_url_tracker_map_entry.h',
-        'browser/google/google_url_tracker_navigation_helper.cc',
         'browser/google/google_url_tracker_navigation_helper.h',
         'browser/google/google_url_tracker_navigation_helper_impl.cc',
         'browser/google/google_url_tracker_navigation_helper_impl.h',