diff options
author | blundell@chromium.org <blundell@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-29 16:32:10 +0000 |
---|---|---|
committer | blundell@chromium.org <blundell@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-29 16:32:10 +0000 |
commit | 57f12776e5aa51ab0b60cd31b12f0acbe3d97207 (patch) | |
tree | f59b17eca74e9542f64aa079952ecd9f65ce0706 | |
parent | 8f783d09bc5d77822300ddad9297d0acd3461835 (diff) | |
download | chromium_src-57f12776e5aa51ab0b60cd31b12f0acbe3d97207.zip chromium_src-57f12776e5aa51ab0b60cd31b12f0acbe3d97207.tar.gz chromium_src-57f12776e5aa51ab0b60cd31b12f0acbe3d97207.tar.bz2 |
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
Review URL: https://codereview.chromium.org/283413002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273490 0039d316-1c4b-4281-b951-d872f2087c98
12 files changed, 188 insertions, 232 deletions
diff --git a/chrome/browser/google/chrome_google_url_tracker_client.cc b/chrome/browser/google/chrome_google_url_tracker_client.cc index 2d8a1e5..5416669 100644 --- a/chrome/browser/google/chrome_google_url_tracker_client.cc +++ b/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()); } diff --git a/chrome/browser/google/google_url_tracker.cc b/chrome/browser/google/google_url_tracker.cc index 62daab0..4983560 100644 --- a/chrome/browser/google/google_url_tracker.cc +++ b/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; } diff --git a/chrome/browser/google/google_url_tracker.h b/chrome/browser/google/google_url_tracker.h index 5e8ba1a..5d1d612 100644 --- a/chrome/browser/google/google_url_tracker.h +++ b/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. diff --git a/chrome/browser/google/google_url_tracker_factory.cc b/chrome/browser/google/google_url_tracker_factory.cc index 239eabc..6c45c85 100644 --- a/chrome/browser/google/google_url_tracker_factory.cc +++ b/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); } diff --git a/chrome/browser/google/google_url_tracker_map_entry.cc b/chrome/browser/google/google_url_tracker_map_entry.cc index e0798fd..ee1ee0f 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, - 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() { diff --git a/chrome/browser/google/google_url_tracker_map_entry.h b/chrome/browser/google/google_url_tracker_map_entry.h index 2e035a7..5f087fb 100644 --- a/chrome/browser/google/google_url_tracker_map_entry.h +++ b/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); }; diff --git a/chrome/browser/google/google_url_tracker_navigation_helper.cc b/chrome/browser/google/google_url_tracker_navigation_helper.cc new file mode 100644 index 0000000..84a2651 --- /dev/null +++ b/chrome/browser/google/google_url_tracker_navigation_helper.cc @@ -0,0 +1,13 @@ +// 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 26b7a53..98804ce 100644 --- a/chrome/browser/google/google_url_tracker_navigation_helper.h +++ b/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_ 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 64e4c5c..99420a8 100644 --- a/chrome/browser/google/google_url_tracker_navigation_helper_impl.cc +++ b/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; } 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 b024744..1d81e54 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: - 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_ diff --git a/chrome/browser/google/google_url_tracker_unittest.cc b/chrome/browser/google/google_url_tracker_unittest.cc index 6b2fedf..0fb975a 100644 --- a/chrome/browser/google/google_url_tracker_unittest.cc +++ b/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)); } @@ -339,6 +314,7 @@ void GoogleURLTrackerTest::SetUp() { void GoogleURLTrackerTest::TearDown() { while (!unique_ids_seen_.empty()) CloseTab(*unique_ids_seen_.begin()); + google_url_tracker_->Shutdown(); } net::TestURLFetcher* GoogleURLTrackerTest::GetFetcher() { @@ -395,8 +371,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 +401,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 +410,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 +435,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 +450,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); } diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 16c92aa..7f2a0ae 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -702,6 +702,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', |