diff options
author | blundell@chromium.org <blundell@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-23 12:10:05 +0000 |
---|---|---|
committer | blundell@chromium.org <blundell@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-23 12:10:05 +0000 |
commit | ed295ecf6dd36dda6b34c88ae84e487130749fd0 (patch) | |
tree | 2bb202d73f10d73a37cd4919234f099f83f61456 | |
parent | 2b1a592a168970a3aa5908df022b2d8966e2dcd4 (diff) | |
download | chromium_src-ed295ecf6dd36dda6b34c88ae84e487130749fd0.zip chromium_src-ed295ecf6dd36dda6b34c88ae84e487130749fd0.tar.gz chromium_src-ed295ecf6dd36dda6b34c88ae84e487130749fd0.tar.bz2 |
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
12 files changed, 232 insertions, 187 deletions
diff --git a/chrome/browser/google/chrome_google_url_tracker_client.cc b/chrome/browser/google/chrome_google_url_tracker_client.cc index 5416669..2d8a1e5 100644 --- a/chrome/browser/google/chrome_google_url_tracker_client.cc +++ b/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()); } diff --git a/chrome/browser/google/google_url_tracker.cc b/chrome/browser/google/google_url_tracker.cc index 4983560..62daab0 100644 --- a/chrome/browser/google/google_url_tracker.cc +++ b/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; } diff --git a/chrome/browser/google/google_url_tracker.h b/chrome/browser/google/google_url_tracker.h index 5d1d612..5e8ba1a 100644 --- a/chrome/browser/google/google_url_tracker.h +++ b/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. diff --git a/chrome/browser/google/google_url_tracker_factory.cc b/chrome/browser/google/google_url_tracker_factory.cc index 6c45c85..239eabc 100644 --- a/chrome/browser/google/google_url_tracker_factory.cc +++ b/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); } diff --git a/chrome/browser/google/google_url_tracker_map_entry.cc b/chrome/browser/google/google_url_tracker_map_entry.cc index ee1ee0f..e0798fd 100644 --- a/chrome/browser/google/google_url_tracker_map_entry.cc +++ b/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() { diff --git a/chrome/browser/google/google_url_tracker_map_entry.h b/chrome/browser/google/google_url_tracker_map_entry.h index 5f087fb..2e035a7 100644 --- a/chrome/browser/google/google_url_tracker_map_entry.h +++ b/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); }; diff --git a/chrome/browser/google/google_url_tracker_navigation_helper.cc b/chrome/browser/google/google_url_tracker_navigation_helper.cc deleted file mode 100644 index 84a2651..0000000 --- a/chrome/browser/google/google_url_tracker_navigation_helper.cc +++ /dev/null @@ -1,13 +0,0 @@ -// 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() { -} diff --git a/chrome/browser/google/google_url_tracker_navigation_helper.h b/chrome/browser/google/google_url_tracker_navigation_helper.h index 98804ce..26b7a53 100644 --- a/chrome/browser/google/google_url_tracker_navigation_helper.h +++ b/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_ diff --git a/chrome/browser/google/google_url_tracker_navigation_helper_impl.cc b/chrome/browser/google/google_url_tracker_navigation_helper_impl.cc index 99420a8..64e4c5c 100644 --- a/chrome/browser/google/google_url_tracker_navigation_helper_impl.cc +++ b/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; } diff --git a/chrome/browser/google/google_url_tracker_navigation_helper_impl.h b/chrome/browser/google/google_url_tracker_navigation_helper_impl.h index 1d81e54..b024744 100644 --- a/chrome/browser/google/google_url_tracker_navigation_helper_impl.h +++ b/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_ diff --git a/chrome/browser/google/google_url_tracker_unittest.cc b/chrome/browser/google/google_url_tracker_unittest.cc index d62eaa6..6b2fedf 100644 --- a/chrome/browser/google/google_url_tracker_unittest.cc +++ b/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); } diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 7d99444..a6bcc17 100644 --- a/chrome/chrome_browser.gypi +++ b/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', |