From c5fb5c113e3193a449e3d77b40f4a98da953e744 Mon Sep 17 00:00:00 2001 From: "joth@chromium.org" Date: Tue, 8 Jun 2010 10:46:01 +0000 Subject: Track options per bridge rather than per dispatcher host Also fixes a bug in arbitrator that updating options might not stop high accuracy providers BUG=40103 TEST=unit_tests --gtest_filter=*Geol* Review URL: http://codereview.chromium.org/2676006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@49155 0039d316-1c4b-4281-b951-d872f2087c98 --- .../geolocation/geolocation_dispatcher_host.cc | 42 +++++++++++++++------ .../geolocation/geolocation_dispatcher_host.h | 12 +++++- chrome/browser/geolocation/location_arbitrator.cc | 43 ++++++++++------------ chrome/browser/geolocation/location_arbitrator.h | 18 +++++++++ .../geolocation/location_arbitrator_unittest.cc | 8 ++++ 5 files changed, 87 insertions(+), 36 deletions(-) (limited to 'chrome/browser') diff --git a/chrome/browser/geolocation/geolocation_dispatcher_host.cc b/chrome/browser/geolocation/geolocation_dispatcher_host.cc index 33d5c2b..c88e41d 100644 --- a/chrome/browser/geolocation/geolocation_dispatcher_host.cc +++ b/chrome/browser/geolocation/geolocation_dispatcher_host.cc @@ -11,6 +11,7 @@ #include "chrome/browser/renderer_host/render_view_host_notification_task.h" #include "chrome/browser/renderer_host/resource_message_filter.h" #include "chrome/common/render_messages.h" +#include "ipc/ipc_message.h" GeolocationDispatcherHost::GeolocationDispatcherHost( int resource_message_filter_process_id, @@ -78,7 +79,8 @@ void GeolocationDispatcherHost::OnUnregisterDispatcher(int render_view_id) { void GeolocationDispatcherHost::OnRequestPermission( int render_view_id, int bridge_id, const GURL& requesting_frame) { - LOG(INFO) << "permission request"; + DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_ + << ":" << render_view_id << ":" << bridge_id; geolocation_permission_context_->RequestGeolocationPermission( resource_message_filter_process_id_, render_view_id, bridge_id, requesting_frame); @@ -86,7 +88,8 @@ void GeolocationDispatcherHost::OnRequestPermission( void GeolocationDispatcherHost::OnCancelPermissionRequest( int render_view_id, int bridge_id, const GURL& requesting_frame) { - LOG(INFO) << "cancel permission request"; + DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_ + << ":" << render_view_id << ":" << bridge_id; geolocation_permission_context_->CancelGeolocationPermissionRequest( resource_message_filter_process_id_, render_view_id, bridge_id, requesting_frame); @@ -98,34 +101,38 @@ void GeolocationDispatcherHost::OnStartUpdating( // WebKit sends the startupdating request before checking permissions, to // optimize the no-location-available case and reduce latency in the success // case (location lookup happens in parallel with the permission request). - LOG(INFO) << "start updating" << render_view_id; + DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_ + << ":" << render_view_id << ":" << bridge_id; + bridge_update_options_[std::make_pair(render_view_id, bridge_id)] = + GeolocationArbitrator::UpdateOptions(enable_high_accuracy); location_arbitrator_ = geolocation_permission_context_->StartUpdatingRequested( resource_message_filter_process_id_, render_view_id, bridge_id, requesting_frame); - DCHECK(location_arbitrator_); - location_arbitrator_->AddObserver( - this, GeolocationArbitrator::UpdateOptions(enable_high_accuracy)); + RefreshUpdateOptions(); } void GeolocationDispatcherHost::OnStopUpdating( int render_view_id, int bridge_id) { - // TODO(joth): Balance calls to RemoveObserver here with AddObserver above. - // http://crbug.com/40103 - LOG(INFO) << "stop updating" << render_view_id; + DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_ + << ":" << render_view_id << ":" << bridge_id; + if (bridge_update_options_.erase(std::make_pair(render_view_id, bridge_id))) + RefreshUpdateOptions(); geolocation_permission_context_->StopUpdatingRequested( resource_message_filter_process_id_, render_view_id, bridge_id); } void GeolocationDispatcherHost::OnSuspend( int render_view_id, int bridge_id) { - LOG(INFO) << "suspend" << render_view_id; + DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_ + << ":" << render_view_id << ":" << bridge_id; // TODO(bulach): connect this with GeolocationArbitrator. } void GeolocationDispatcherHost::OnResume( int render_view_id, int bridge_id) { - LOG(INFO) << "resume" << render_view_id; + DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_ + << ":" << render_view_id << ":" << bridge_id; // TODO(bulach): connect this with GeolocationArbitrator. } @@ -162,3 +169,16 @@ void GeolocationDispatcherHost::UnregisterDispatcher( DCHECK_EQ(1u, geolocation_renderer_ids_.count(render_view_id)); geolocation_renderer_ids_.erase(render_view_id); } + +void GeolocationDispatcherHost::RefreshUpdateOptions() { + DCHECK(location_arbitrator_); + if (bridge_update_options_.empty()) { + location_arbitrator_->RemoveObserver(this); + location_arbitrator_ = NULL; + } else { + location_arbitrator_->AddObserver( + this, + GeolocationArbitrator::UpdateOptions::Collapse( + bridge_update_options_)); + } +} diff --git a/chrome/browser/geolocation/geolocation_dispatcher_host.h b/chrome/browser/geolocation/geolocation_dispatcher_host.h index 6cff179..31da3a6 100644 --- a/chrome/browser/geolocation/geolocation_dispatcher_host.h +++ b/chrome/browser/geolocation/geolocation_dispatcher_host.h @@ -5,18 +5,20 @@ #ifndef CHROME_BROWSER_GEOLOCATION_GEOLOCATION_DISPATCHER_HOST_H_ #define CHROME_BROWSER_GEOLOCATION_GEOLOCATION_DISPATCHER_HOST_H_ +#include #include +#include #include "base/basictypes.h" #include "base/ref_counted.h" #include "chrome/browser/geolocation/location_arbitrator.h" -#include "ipc/ipc_message.h" class GeolocationPermissionContext; class GURL; class ResourceMessageFilter; class URLRequestContextGetter; struct Geoposition; +namespace IPC { class Message; } // GeolocationDispatcherHost is a delegate for Geolocation messages used by // ResourceMessageFilter. @@ -57,6 +59,9 @@ class GeolocationDispatcherHost // UI thread if not already in there. void RegisterDispatcher(int process_id, int render_view_id); void UnregisterDispatcher(int process_id, int render_view_id); + // Updates the |location_arbitrator_| with the currently required update + // options, based on |bridge_update_options_|. + void RefreshUpdateOptions(); int resource_message_filter_process_id_; scoped_refptr geolocation_permission_context_; @@ -66,6 +71,11 @@ class GeolocationDispatcherHost // context switches. // Only used on the IO thread. std::set geolocation_renderer_ids_; + // Maps to the location arbitrator update options + // that correspond to this particular bridge. + typedef std::map, GeolocationArbitrator::UpdateOptions> + BridgeUpdateOptionsMap; + BridgeUpdateOptionsMap bridge_update_options_; // Only set whilst we are registered with the arbitrator. scoped_refptr location_arbitrator_; diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc index 2776ec7..810beb2 100644 --- a/chrome/browser/geolocation/location_arbitrator.cc +++ b/chrome/browser/geolocation/location_arbitrator.cc @@ -65,8 +65,7 @@ class GeolocationArbitratorImpl void RegisterProvider(LocationProviderBase* provider, ScopedVector* provider_vector); void ResetProviders(ScopedVector* providers); - void CreateProvidersIfRequired(); - bool HasHighAccuracyObserver(); + void ModifyProvidersIfRequired(); // Returns true if |new_position| is an improvement over |old_position|. // Set |from_same_provider| to true if both the positions came from the same @@ -136,8 +135,8 @@ GeolocationArbitratorImpl::GeolocationArbitratorImpl( GeolocationArbitratorImpl::~GeolocationArbitratorImpl() { DCHECK(CalledOnValidThread()); DCHECK(observers_.empty()) << "Not all observers have unregistered"; - ResetProviders(&gps_providers_); - ResetProviders(&network_providers_); + observers_.clear(); + ModifyProvidersIfRequired(); DCHECK(this == g_instance_); g_instance_ = NULL; } @@ -147,7 +146,7 @@ void GeolocationArbitratorImpl::AddObserver( const UpdateOptions& update_options) { DCHECK(CalledOnValidThread()); observers_[delegate] = update_options; - CreateProvidersIfRequired(); + ModifyProvidersIfRequired(); if (position_.IsInitialized()) { delegate->OnLocationUpdate(position_); @@ -158,11 +157,7 @@ bool GeolocationArbitratorImpl::RemoveObserver( GeolocationArbitrator::Delegate* delegate) { DCHECK(CalledOnValidThread()); size_t remove = observers_.erase(delegate); - // TODO(joth): Delayed callback to linger before destroying providers. - if (!HasHighAccuracyObserver()) - ResetProviders(&gps_providers_); - if (observers_.empty()) - ResetProviders(&network_providers_); + ModifyProvidersIfRequired(); return remove > 0; } @@ -260,9 +255,15 @@ void GeolocationArbitratorImpl::ResetProviders( providers->reset(); } -void GeolocationArbitratorImpl::CreateProvidersIfRequired() { +void GeolocationArbitratorImpl::ModifyProvidersIfRequired() { DCHECK(CalledOnValidThread()); - DCHECK(!observers_.empty()); + // TODO(joth): Delayed callback to linger before destroying providers. + if (observers_.empty()) { + ResetProviders(&network_providers_); + ResetProviders(&gps_providers_); + return; + } + if (network_providers_.empty() && !request_consumer_.HasPendingRequests()) { // There are no network providers either created or pending creation. access_token_store_->LoadAccessTokens( @@ -270,20 +271,14 @@ void GeolocationArbitratorImpl::CreateProvidersIfRequired() { NewCallback(this, &GeolocationArbitratorImpl::OnAccessTokenStoresLoaded)); } - if (gps_providers_.empty() && HasHighAccuracyObserver()) { - RegisterProvider(provider_factory_->NewGpsLocationProvider(), - &gps_providers_); - } -} -bool GeolocationArbitratorImpl::HasHighAccuracyObserver() { - DCHECK(CalledOnValidThread()); - for (DelegateMap::const_iterator it = observers_.begin(); - it != observers_.end(); ++it) { - if (it->second.use_high_accuracy) - return true; + if (UpdateOptions::Collapse(observers_).use_high_accuracy) { + if (gps_providers_.empty()) + RegisterProvider(provider_factory_->NewGpsLocationProvider(), + &gps_providers_); + } else { + ResetProviders(&gps_providers_); } - return false; } bool GeolocationArbitratorImpl::IsNewPositionBetter( diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h index 085e320..74403b4 100644 --- a/chrome/browser/geolocation/location_arbitrator.h +++ b/chrome/browser/geolocation/location_arbitrator.h @@ -70,12 +70,30 @@ class GeolocationArbitrator : public base::RefCounted { virtual void OnLocationUpdate(const Geoposition& position) = 0; protected: + Delegate() {} virtual ~Delegate() {} + + private: + DISALLOW_COPY_AND_ASSIGN(Delegate); }; struct UpdateOptions { UpdateOptions() : use_high_accuracy(false) {} explicit UpdateOptions(bool high_accuracy) : use_high_accuracy(high_accuracy) {} + + // Given a map this function will iterate the map + // and collapse all the options found to a single instance that satisfies + // them all. + template + static UpdateOptions Collapse(const MAP& options_map) { + for (typename MAP::const_iterator it = options_map.begin(); + it != options_map.end(); ++it) { + if (it->second.use_high_accuracy) + return UpdateOptions(true); + } + return UpdateOptions(false); + } + bool use_high_accuracy; }; diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc index be29d50..2a4eee2 100644 --- a/chrome/browser/geolocation/location_arbitrator_unittest.cc +++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc @@ -210,11 +210,19 @@ TEST_F(GeolocationLocationArbitratorTest, MockLocationObserver observer; arbitrator_->AddObserver( &observer, GeolocationArbitrator::UpdateOptions(false)); + access_token_store_->NotifyDelegateTokensLoaded(); + EXPECT_TRUE(providers_->cell_); EXPECT_FALSE(providers_->gps_); arbitrator_->AddObserver( &observer, GeolocationArbitrator::UpdateOptions(true)); + EXPECT_TRUE(providers_->cell_); EXPECT_TRUE(providers_->gps_); + arbitrator_->AddObserver( + &observer, GeolocationArbitrator::UpdateOptions(false)); + EXPECT_TRUE(providers_->cell_); + EXPECT_FALSE(providers_->gps_); EXPECT_TRUE(arbitrator_->RemoveObserver(&observer)); + EXPECT_FALSE(providers_->cell_); EXPECT_FALSE(arbitrator_->RemoveObserver(&observer)); } -- cgit v1.1