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

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

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

chrome/browser/google/chrome_google_url_tracker_client.cc

@@ -6,6 +6,7 @@
 
 #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"
@@ -48,12 +49,13 @@ 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(
-        controller,
+        scoped_ptr<GoogleURLTrackerNavigationHelper>(
+            new GoogleURLTrackerNavigationHelperImpl(
+                controller->GetWebContents(), google_url_tracker())),
         infobar_service,
         controller->GetPendingEntry()->GetUniqueID());
   }

chrome/browser/google/google_url_tracker.cc

@@ -33,14 +33,11 @@ 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,
-    scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
-    Mode mode)
+GoogleURLTracker::GoogleURLTracker(Profile* profile,
+                                   scoped_ptr<GoogleURLTrackerClient> client,
+                                   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 :
@@ -54,7 +51,6 @@ GoogleURLTracker::GoogleURLTracker(
       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
@@ -205,7 +201,6 @@ void GoogleURLTracker::OnIPAddressChanged() {
 
 void GoogleURLTracker::Shutdown() {
   client_.reset();
-  nav_helper_.reset();
   fetcher_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
   net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
@@ -219,7 +214,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;
 }
@@ -281,39 +276,41 @@ void GoogleURLTracker::SearchCommitted() {
 }
 
 void GoogleURLTracker::OnNavigationPending(
-    content::NavigationController* navigation_controller,
+    scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
     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;
-    // 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()) {
+    if (!map_entry) {
       // 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.
-      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()) {
+      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()) {
       // This is a new search on a tab where we already have an infobar.
-      i->second->infobar_delegate()->set_pending_id(pending_id);
+      map_entry->infobar_delegate()->set_pending_id(pending_id);
     }
-  } else if (i != entry_map_.end()){
-    if (i->second->has_infobar_delegate()) {
+
+    // 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()) {
       // 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
@@ -323,13 +320,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(*i->second, false);
-      i->second->infobar_delegate()->set_pending_id(0);
+      UnregisterForEntrySpecificNotifications(map_entry, false);
+      map_entry->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.
-      i->second->Close(false);
+      map_entry->Close(false);
     }
   } else {
     // Non-search navigation on a tab without an infobars.  This is irrelevant
@@ -344,7 +341,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 {
@@ -360,7 +357,7 @@ void GoogleURLTracker::OnNavigationCommitted(InfoBarService* infobar_service,
 }
 
 void GoogleURLTracker::OnTabClosed(
-    content::NavigationController* navigation_controller) {
+    GoogleURLTrackerNavigationHelper* nav_helper) {
   // 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
@@ -370,7 +367,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_controller() == navigation_controller) {
+    if (i->second->navigation_helper() == nav_helper) {
       i->second->Close(false);
       return;
     }
@@ -390,26 +387,22 @@ void GoogleURLTracker::CloseAllEntries(bool redo_searches) {
 }
 
 void GoogleURLTracker::UnregisterForEntrySpecificNotifications(
-    const GoogleURLTrackerMapEntry& map_entry,
+    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 (nav_helper_->IsListeningForNavigationCommit(
-          map_entry.navigation_controller())) {
-    nav_helper_->SetListeningForNavigationCommit(
-        map_entry.navigation_controller(), false);
+  if (map_entry->navigation_helper()->IsListeningForNavigationCommit()) {
+    map_entry->navigation_helper()->SetListeningForNavigationCommit(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 =
-      nav_helper_->IsListeningForTabDestruction(
-          map_entry.navigation_controller());
-  DCHECK_NE(registered_for_tab_destruction, map_entry.has_infobar_delegate());
+      map_entry->navigation_helper()->IsListeningForTabDestruction();
+  DCHECK_NE(registered_for_tab_destruction, map_entry->has_infobar_delegate());
   if (registered_for_tab_destruction) {
-    nav_helper_->SetListeningForTabDestruction(
-        map_entry.navigation_controller(), false);
+    map_entry->navigation_helper()->SetListeningForTabDestruction(false);
   }
 
   // Our global listeners for these other notifications should be in place iff
@@ -419,8 +412,7 @@ void GoogleURLTracker::UnregisterForEntrySpecificNotifications(
   // See the various cases inside OnNavigationPending().
   for (EntryMap::const_iterator i(entry_map_.begin()); i != entry_map_.end();
        ++i) {
-    if (nav_helper_->IsListeningForNavigationCommit(
-            i->second->navigation_controller())) {
+    if (i->second->navigation_helper()->IsListeningForNavigationCommit()) {
       DCHECK(client_->IsListeningForNavigationStart());
       return;
     }

chrome/browser/google/google_url_tracker.h

@@ -26,10 +26,6 @@ class GoogleURLTrackerNavigationHelper;
 class PrefService;
 class Profile;
 
-namespace content {
-class NavigationController;
-}
-
 namespace infobars {
 class InfoBar;
 }
@@ -72,7 +68,6 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // GoogleURLTrackerFactory::GetForProfile().
   GoogleURLTracker(Profile* profile,
                    scoped_ptr<GoogleURLTrackerClient> client,
-                   scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
                    Mode mode);
 
   virtual ~GoogleURLTracker();
@@ -113,14 +108,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.
-  // |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
+  // |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
   // 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(
-      content::NavigationController* navigation_controller,
+      scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
       InfoBarService* infobar_service,
       int pending_id);
 
@@ -131,8 +126,7 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
                                      const GURL& search_url);
 
   // Called by the navigation observer when a tab closes.
-  virtual void OnTabClosed(
-      content::NavigationController* navigation_controller);
+  virtual void OnTabClosed(GoogleURLTrackerNavigationHelper* nav_helper);
 
   scoped_ptr<Subscription> RegisterCallback(
       const OnGoogleURLUpdatedCallback& cb);
@@ -174,14 +168,14 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // Google TLD.
   void CloseAllEntries(bool redo_searches);
 
-  // Unregisters any listeners for the navigation controller in |map_entry|.
+  // Unregisters any listeners for the navigation helper 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(
-      const GoogleURLTrackerMapEntry& map_entry,
+      GoogleURLTrackerMapEntry* map_entry,
       bool must_be_listening_for_commit);
 
   void NotifyGoogleURLUpdated(GURL old_url, GURL new_url);
@@ -192,8 +186,6 @@ 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,7 +7,6 @@
 #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"
@@ -38,11 +37,8 @@ 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,
-    const content::NavigationController* navigation_controller)
+    scoped_ptr<GoogleURLTrackerNavigationHelper> navigation_helper)
     : google_url_tracker_(google_url_tracker),
       infobar_service_(infobar_service),
       infobar_delegate_(NULL),
-      navigation_controller_(navigation_controller) {
+      navigation_helper_(navigation_helper.Pass()) {
 }
 
 GoogleURLTrackerMapEntry::~GoogleURLTrackerMapEntry() {

chrome/browser/google/google_url_tracker_map_entry.h

@@ -5,6 +5,8 @@
 #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"
 
@@ -12,16 +14,12 @@ class GoogleURLTracker;
 class GoogleURLTrackerInfoBarDelegate;
 class InfoBarService;
 
-namespace content {
-class NavigationController;
-}
-
 class GoogleURLTrackerMapEntry : public content::NotificationObserver {
  public:
   GoogleURLTrackerMapEntry(
       GoogleURLTracker* google_url_tracker,
       InfoBarService* infobar_service,
-      const content::NavigationController* navigation_controller);
+      scoped_ptr<GoogleURLTrackerNavigationHelper> navigation_helper);
   virtual ~GoogleURLTrackerMapEntry();
 
   bool has_infobar_delegate() const { return !!infobar_delegate_; }
@@ -30,8 +28,8 @@ class GoogleURLTrackerMapEntry : public content::NotificationObserver {
   }
   void SetInfoBarDelegate(GoogleURLTrackerInfoBarDelegate* infobar_delegate);
 
-  const content::NavigationController* navigation_controller() const {
-    return navigation_controller_;
+  GoogleURLTrackerNavigationHelper* navigation_helper() {
+    return navigation_helper_.get();
   }
 
   void Close(bool redo_search);
@@ -48,7 +46,7 @@ class GoogleURLTrackerMapEntry : public content::NotificationObserver {
   GoogleURLTracker* const google_url_tracker_;
   const InfoBarService* const infobar_service_;
   GoogleURLTrackerInfoBarDelegate* infobar_delegate_;
-  const content::NavigationController* const navigation_controller_;
+  scoped_ptr<GoogleURLTrackerNavigationHelper> navigation_helper_;
 
   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,46 +5,42 @@
 #ifndef CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_H_
 #define CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_H_
 
-class GoogleURLTracker;
-class InfoBarService;
-class Profile;
+#include "base/macros.h"
 
-namespace content {
-class NavigationController;
-}
+class GoogleURLTracker;
 
 // A helper class for GoogleURLTracker that abstracts the details of listening
 // for various navigation events.
 class GoogleURLTrackerNavigationHelper {
  public:
-  virtual ~GoogleURLTrackerNavigationHelper() {}
+  explicit GoogleURLTrackerNavigationHelper(
+      GoogleURLTracker* google_url_tracker);
+  virtual ~GoogleURLTrackerNavigationHelper();
 
-  // Sets the GoogleURLTracker that is associated with this object.
-  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) = 0;
+  // 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;
 
-  // 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 navigation
+  // commits.
+  virtual bool IsListeningForNavigationCommit() = 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;
+  // 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 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;
+  // Returns whether or not this object is currently listening for tab
+  // destruction.
+  virtual bool IsListeningForTabDestruction() = 0;
+
+ protected:
+  GoogleURLTracker* google_url_tracker() { return google_url_tracker_; }
+
+ private:
+  GoogleURLTracker* google_url_tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerNavigationHelper);
 };
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_H_

chrome/browser/google/google_url_tracker_navigation_helper_impl.cc

@@ -12,25 +12,21 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 
-GoogleURLTrackerNavigationHelperImpl::
-    GoogleURLTrackerNavigationHelperImpl() : tracker_(NULL) {
+GoogleURLTrackerNavigationHelperImpl::GoogleURLTrackerNavigationHelperImpl(
+    content::WebContents* web_contents,
+    GoogleURLTracker* tracker)
+    : GoogleURLTrackerNavigationHelper(tracker),
+      web_contents_(web_contents) {
 }
 
-GoogleURLTrackerNavigationHelperImpl::
-    ~GoogleURLTrackerNavigationHelperImpl() {
-}
-
-void GoogleURLTrackerNavigationHelperImpl::SetGoogleURLTracker(
-    GoogleURLTracker* tracker) {
-  DCHECK(tracker);
-  tracker_ = tracker;
+GoogleURLTrackerNavigationHelperImpl::~GoogleURLTrackerNavigationHelperImpl() {
 }
 
 void GoogleURLTrackerNavigationHelperImpl::SetListeningForNavigationCommit(
-    const content::NavigationController* nav_controller,
     bool listen) {
   content::NotificationSource navigation_controller_source =
-      content::Source<content::NavigationController>(nav_controller);
+      content::Source<content::NavigationController>(
+          &web_contents_->GetController());
   if (listen) {
     registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                     navigation_controller_source);
@@ -40,47 +36,35 @@ void GoogleURLTrackerNavigationHelperImpl::SetListeningForNavigationCommit(
   }
 }
 
-bool GoogleURLTrackerNavigationHelperImpl::IsListeningForNavigationCommit(
-    const content::NavigationController* nav_controller) {
+bool GoogleURLTrackerNavigationHelperImpl::IsListeningForNavigationCommit() {
   content::NotificationSource navigation_controller_source =
-      content::Source<content::NavigationController>(nav_controller);
+      content::Source<content::NavigationController>(
+          &web_contents_->GetController());
   return registrar_.IsRegistered(
       this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
       navigation_controller_source);
 }
 
 void GoogleURLTrackerNavigationHelperImpl::SetListeningForTabDestruction(
-    const content::NavigationController* nav_controller,
     bool listen) {
-  content::NotificationSource navigation_controller_source =
-      content::Source<content::NavigationController>(nav_controller);
+  content::NotificationSource web_contents_source =
+      content::Source<content::WebContents>(web_contents_);
   if (listen) {
-    registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                   GetWebContentsSource(navigation_controller_source));
+    registrar_.Add(this,
+                   content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                   web_contents_source);
   } else {
-    registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                      GetWebContentsSource(navigation_controller_source));
+    registrar_.Remove(this,
+                      content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                      web_contents_source);
   }
 }
 
-bool GoogleURLTrackerNavigationHelperImpl::IsListeningForTabDestruction(
-    const content::NavigationController* nav_controller) {
-  content::NotificationSource navigation_controller_source =
-      content::Source<content::NavigationController>(nav_controller);
+bool GoogleURLTrackerNavigationHelperImpl::IsListeningForTabDestruction() {
   return registrar_.IsRegistered(
       this,
       content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-      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);
+      content::Source<content::WebContents>(web_contents_));
 }
 
 void GoogleURLTrackerNavigationHelperImpl::Observe(
@@ -91,23 +75,24 @@ 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.
-        tracker_->OnTabClosed(controller);
-      tracker_->OnNavigationCommitted(infobar_service, search_url);
+        google_url_tracker()->OnTabClosed(this);
+      google_url_tracker()->OnNavigationCommitted(infobar_service, search_url);
       break;
     }
 
     case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
-      content::WebContents* web_contents =
-          content::Source<content::WebContents>(source).ptr();
-      tracker_->OnTabClosed(&web_contents->GetController());
+      DCHECK_EQ(web_contents_,
+                content::Source<content::WebContents>(source).ptr());
+      google_url_tracker()->OnTabClosed(this);
       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:
-  explicit GoogleURLTrackerNavigationHelperImpl();
+  GoogleURLTrackerNavigationHelperImpl(content::WebContents* web_contents,
+                                       GoogleURLTracker* tracker);
   virtual ~GoogleURLTrackerNavigationHelperImpl();
 
-  // GoogleURLTrackerNavigationHelper.
-  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
+  // GoogleURLTrackerNavigationHelper:
   virtual void SetListeningForNavigationCommit(
-      const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
-  virtual bool IsListeningForNavigationCommit(
-      const content::NavigationController* nav_controller) OVERRIDE;
+  virtual bool IsListeningForNavigationCommit() OVERRIDE;
   virtual void SetListeningForTabDestruction(
-      const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
-  virtual bool IsListeningForTabDestruction(
-      const content::NavigationController* nav_controller) OVERRIDE;
+  virtual bool IsListeningForTabDestruction() OVERRIDE;
 
  private:
   // content::NotificationObserver:
@@ -36,13 +36,10 @@ class GoogleURLTrackerNavigationHelperImpl
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // Returns a WebContents NavigationSource for the WebContents corresponding to
-  // the given NavigationController NotificationSource.
-  virtual content::NotificationSource GetWebContentsSource(
-      const content::NotificationSource& nav_controller_source);
-
-  GoogleURLTracker* tracker_;
+  content::WebContents* web_contents_;
   content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerNavigationHelperImpl);
 };
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_NAVIGATION_HELPER_IMPL_H_

chrome/browser/google/google_url_tracker_unittest.cc

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

chrome/chrome_browser.gypi

@@ -696,6 +696,7 @@
         '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',