diff options
14 files changed, 280 insertions, 108 deletions
diff --git a/chrome/browser/geolocation/geolocation_browsertest.cc b/chrome/browser/geolocation/geolocation_browsertest.cc index 9f871a8..062829f 100644 --- a/chrome/browser/geolocation/geolocation_browsertest.cc +++ b/chrome/browser/geolocation/geolocation_browsertest.cc @@ -199,7 +199,7 @@ class GeolocationBrowserTest : public InProcessBrowserTest { void Initialize(InitializationOptions options) { GeolocationArbitrator::SetProviderFactoryForTest( - &NewAutoSuccessMockLocationProvider); + &NewAutoSuccessMockNetworkLocationProvider); if (!server_.get()) server_ = StartHTTPServer(); @@ -324,7 +324,7 @@ class GeolocationBrowserTest : public InProcessBrowserTest { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_DisplaysPermissionBar DISABLED_DisplaysPermissionBar #else #define MAYBE_DisplaysPermissionBar DisplaysPermissionBar @@ -337,7 +337,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_DisplaysPermissionBar) { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_Geoposition DISABLED_Geoposition #else #define MAYBE_Geoposition Geoposition @@ -352,7 +352,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_Geoposition) { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_ErrorOnPermissionDenied DISABLED_ErrorOnPermissionDenied #else #define MAYBE_ErrorOnPermissionDenied ErrorOnPermissionDenied @@ -368,7 +368,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_ErrorOnPermissionDenied) { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_NoInfobarForSecondTab DISABLED_NoInfobarForSecondTab #else #define MAYBE_NoInfobarForSecondTab NoInfobarForSecondTab @@ -378,6 +378,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_NoInfobarForSecondTab) { Initialize(INITIALIZATION_NONE); AddGeolocationWatch(true); SetInfobarResponse(current_url_, true); + // Checks infobar will not be created a second tab. Initialize(INITIALIZATION_NEWTAB); AddGeolocationWatch(false); @@ -386,7 +387,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_NoInfobarForSecondTab) { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_NoInfobarForDeniedOrigin DISABLED_NoInfobarForDeniedOrigin #else #define MAYBE_NoInfobarForDeniedOrigin NoInfobarForDeniedOrigin @@ -407,7 +408,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_NoInfobarForDeniedOrigin) { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_NoInfobarForAllowedOrigin DISABLED_NoInfobarForAllowedOrigin #else #define MAYBE_NoInfobarForAllowedOrigin NoInfobarForAllowedOrigin @@ -425,7 +426,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_NoInfobarForOffTheRecord DISABLED_NoInfobarForOffTheRecord #else #define MAYBE_NoInfobarForOffTheRecord NoInfobarForOffTheRecord @@ -448,7 +449,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, MAYBE_NoInfobarForOffTheRecord) { #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_IFramesWithFreshPosition DISABLED_IFramesWithFreshPosition #else // TODO(bulach): investigate this failure. @@ -498,7 +499,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, #if defined(OS_MACOSX) // TODO(bulach): investigate why this fails on mac. It may be related to: -// http://crbug.com//29424 +// http://crbug.com/29424 #define MAYBE_IFramesWithCachedPosition DISABLED_IFramesWithCachedPosition #else // TODO(bulach): enable this test when we roll to diff --git a/chrome/browser/geolocation/geolocation_permission_context.cc b/chrome/browser/geolocation/geolocation_permission_context.cc index 5ff17c0..d78b5a7 100644 --- a/chrome/browser/geolocation/geolocation_permission_context.cc +++ b/chrome/browser/geolocation/geolocation_permission_context.cc @@ -174,9 +174,19 @@ GeolocationArbitrator* GeolocationPermissionContext::StartUpdatingRequested( int render_process_id, int render_view_id, int bridge_id, const GURL& requesting_frame) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); - // TODO(joth): Use requesting_frame parameter to short-circuit the latched - // permission-denied case, and so avoid starting up location arbitrator. - return GeolocationArbitrator::GetInstance(); + // Note we cannot store the arbitrator as a member as it is not thread safe. + GeolocationArbitrator* arbitrator = GeolocationArbitrator::GetInstance(); + + // WebKit will not request permsission until it has received a valid + // location, but the google network location provider will not give a + // valid location until the user has granted permission. So we cut the Gordian + // Knot by reusing the the 'start updating' request to also trigger + // a 'permission request' should the provider still be awaiting permission. + if (!arbitrator->HasPermissionBeenGranted()) { + RequestGeolocationPermission(render_process_id, render_view_id, bridge_id, + requesting_frame); + } + return arbitrator; } void GeolocationPermissionContext::RequestPermissionFromUI( @@ -215,4 +225,17 @@ void GeolocationPermissionContext::NotifyPermissionSet( &RenderViewHost::Send, new ViewMsg_Geolocation_PermissionSet(render_view_id, bridge_id, allowed)); + if (allowed) { + ChromeThread::PostTask( + ChromeThread::IO, FROM_HERE, + NewRunnableMethod(this, + &GeolocationPermissionContext::NotifyArbitratorPermissionGranted, + requesting_frame)); + } +} + +void GeolocationPermissionContext::NotifyArbitratorPermissionGranted( + const GURL& requesting_frame) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); + GeolocationArbitrator::GetInstance()->OnPermissionGranted(requesting_frame); } diff --git a/chrome/browser/geolocation/geolocation_permission_context.h b/chrome/browser/geolocation/geolocation_permission_context.h index 8c2d264..a7c31fa 100644 --- a/chrome/browser/geolocation/geolocation_permission_context.h +++ b/chrome/browser/geolocation/geolocation_permission_context.h @@ -62,6 +62,9 @@ class GeolocationPermissionContext int render_process_id, int render_view_id, int bridge_id, const GURL& requesting_frame, bool allowed); + // Calls GeolocationArbitrator::OnPermissionGranted. + void NotifyArbitratorPermissionGranted(const GURL& requesting_frame); + // This should only be accessed from the UI thread. Profile* const profile_; diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc index 4b42dd7..4f46799 100644 --- a/chrome/browser/geolocation/location_arbitrator.cc +++ b/chrome/browser/geolocation/location_arbitrator.cc @@ -38,6 +38,8 @@ class GeolocationArbitratorImpl virtual void AddObserver(GeolocationArbitrator::Delegate* delegate, const UpdateOptions& update_options); virtual bool RemoveObserver(GeolocationArbitrator::Delegate* delegate); + virtual void OnPermissionGranted(const GURL& requesting_frame); + virtual bool HasPermissionBeenGranted() const; // ListenerInterface virtual void LocationUpdateAvailable(LocationProviderBase* provider); @@ -62,6 +64,8 @@ class GeolocationArbitratorImpl // The current best estimate of our position. Geoposition position_; + GURL most_recent_authorized_frame_; + CancelableRequestConsumer request_consumer_; }; @@ -107,6 +111,19 @@ bool GeolocationArbitratorImpl::RemoveObserver( return remove > 0; } +void GeolocationArbitratorImpl::OnPermissionGranted( + const GURL& requesting_frame) { + DCHECK(CalledOnValidThread()); + most_recent_authorized_frame_ = requesting_frame; + if (provider_ != NULL) + provider_->OnPermissionGranted(requesting_frame); +} + +bool GeolocationArbitratorImpl::HasPermissionBeenGranted() const { + DCHECK(CalledOnValidThread()); + return most_recent_authorized_frame_.is_valid(); +} + void GeolocationArbitratorImpl::LocationUpdateAvailable( LocationProviderBase* provider) { DCHECK(CalledOnValidThread()); @@ -136,21 +153,18 @@ void GeolocationArbitratorImpl::OnAccessTokenStoresLoaded( if (g_provider_factory_function_for_test) { provider_.reset(g_provider_factory_function_for_test()); } else { - // TODO(joth): Once we have arbitration implementation, iterate the whole + // TODO(joth): When arbitration is implemented, iterate the whole // set rather than cherry-pick our defaul url. - const AccessTokenStore::AccessTokenSet::const_iterator token = - access_token_set.find(default_url_); - // TODO(joth): Follow up with GLS folks if they have a plan for replacing - // the hostname field. Sending chromium.org is a stop-gap. provider_.reset(NewNetworkLocationProvider( access_token_store_.get(), context_getter_.get(), default_url_, - token != access_token_set.end() ? token->second : string16(), - ASCIIToUTF16("chromium.org"))); + access_token_set[default_url_])); } DCHECK(provider_ != NULL); provider_->RegisterListener(this); const bool ok = provider_->StartProvider(); DCHECK(ok); + if (most_recent_authorized_frame_.is_valid()) + provider_->OnPermissionGranted(most_recent_authorized_frame_); } void GeolocationArbitratorImpl::CreateProviders() { diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h index 98477c7..4742e56 100644 --- a/chrome/browser/geolocation/location_arbitrator.h +++ b/chrome/browser/geolocation/location_arbitrator.h @@ -8,6 +8,7 @@ #include "base/ref_counted.h" class AccessTokenStore; +class GURL; class LocationProviderBase; class URLRequestContextGetter; struct Geoposition; @@ -63,6 +64,17 @@ class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> { // via AddObserver(). Returns true if the observer was removed. virtual bool RemoveObserver(Delegate* delegate) = 0; + // Called everytime permission is granted to a page for using geolocation. + // This may either be through explicit user action (e.g. responding to the + // infobar prompt) or inferred from a persisted site permission. + // The arbitrator will inform all providers of this, which may in turn use + // this information to modify their internal policy. + virtual void OnPermissionGranted(const GURL& requesting_frame) = 0; + + // Returns true if this arbitrator has received at least one call to + // OnPermissionGranted(). + virtual bool HasPermissionBeenGranted() const = 0; + // For testing, a factory functino can be set which will be used to create // a specified test provider. Pass NULL to reset to the default behavior. typedef LocationProviderBase* (*LocationProviderFactoryFunction)(void); diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc index 6f3bf72..38dd9da 100644 --- a/chrome/browser/geolocation/location_arbitrator_unittest.cc +++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc @@ -62,6 +62,15 @@ TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) { SUCCEED(); } +TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) { + EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted()); + arbitrator_->OnPermissionGranted(GURL("http://frame.test")); + EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted()); + // Can't check the provider has been notified without going through the + // motions to create the provider (see next test). + EXPECT_FALSE(MockLocationProvider::instance_); +} + TEST_F(GeolocationLocationArbitratorTest, NormalUsage) { ASSERT_TRUE(access_token_store_); ASSERT_TRUE(arbitrator_ != NULL); @@ -87,6 +96,17 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) { EXPECT_EQ(MockLocationProvider::instance_->position_.latitude, observer.last_position_.latitude); + EXPECT_FALSE( + MockLocationProvider::instance_->permission_granted_url_.is_valid()); + EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted()); + GURL frame_url("http://frame.test"); + arbitrator_->OnPermissionGranted(frame_url); + EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted()); + EXPECT_TRUE( + MockLocationProvider::instance_->permission_granted_url_.is_valid()); + EXPECT_EQ(frame_url, + MockLocationProvider::instance_->permission_granted_url_); + EXPECT_TRUE(arbitrator_->RemoveObserver(&observer)); } diff --git a/chrome/browser/geolocation/location_provider.h b/chrome/browser/geolocation/location_provider.h index 8ac1c8f..6d8d6a7 100644 --- a/chrome/browser/geolocation/location_provider.h +++ b/chrome/browser/geolocation/location_provider.h @@ -68,6 +68,9 @@ class LocationProviderBase : public NonThreadSafe { // Provides a hint to the provider that new location data is needed as soon // as possible. Default implementation does nothing. virtual void UpdatePosition() {} + // Delegated to the provider by GeolocationArbitrator. See the corresponding + // method on that class for more details. + virtual void OnPermissionGranted(const GURL& requesting_frame) {} bool has_listeners() const; @@ -94,7 +97,6 @@ LocationProviderBase* NewNetworkLocationProvider( AccessTokenStore* access_token_store, URLRequestContextGetter* context, const GURL& url, - const string16& access_token, - const string16& host_name); + const string16& access_token); #endif // CHROME_BROWSER_GEOLOCATION_LOCATION_PROVIDER_H_ diff --git a/chrome/browser/geolocation/mock_location_provider.cc b/chrome/browser/geolocation/mock_location_provider.cc index 7851f30..7a7bb9c 100644 --- a/chrome/browser/geolocation/mock_location_provider.cc +++ b/chrome/browser/geolocation/mock_location_provider.cc @@ -34,12 +34,18 @@ void MockLocationProvider::GetPosition(Geoposition* position) { *position = position_; } +void MockLocationProvider::OnPermissionGranted(const GURL& requesting_frame) { + permission_granted_url_ = requesting_frame; +} + // Mock location provider that automatically calls back it's client when // StartProvider is called. class AutoMockLocationProvider : public MockLocationProvider { public: - explicit AutoMockLocationProvider(bool has_valid_location) - : ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) { + AutoMockLocationProvider(bool has_valid_location, + bool requires_permission_to_start) + : ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)), + requires_permission_to_start_(requires_permission_to_start) { if (has_valid_location) { position_.accuracy = 3; position_.latitude = 4.3; @@ -51,13 +57,25 @@ class AutoMockLocationProvider : public MockLocationProvider { } virtual bool StartProvider() { MockLocationProvider::StartProvider(); - MessageLoop::current()->PostTask( - FROM_HERE, task_factory_.NewRunnableMethod( - &MockLocationProvider::UpdateListeners)); + if (!requires_permission_to_start_) { + MessageLoop::current()->PostTask( + FROM_HERE, task_factory_.NewRunnableMethod( + &MockLocationProvider::UpdateListeners)); + } return true; } + void OnPermissionGranted(const GURL& requesting_frame) { + MockLocationProvider::OnPermissionGranted(requesting_frame); + if (requires_permission_to_start_) { + MessageLoop::current()->PostTask( + FROM_HERE, task_factory_.NewRunnableMethod( + &MockLocationProvider::UpdateListeners)); + } + } + ScopedRunnableMethodFactory<MockLocationProvider> task_factory_; + const bool requires_permission_to_start_; }; LocationProviderBase* NewMockLocationProvider() { @@ -65,9 +83,13 @@ LocationProviderBase* NewMockLocationProvider() { } LocationProviderBase* NewAutoSuccessMockLocationProvider() { - return new AutoMockLocationProvider(true); + return new AutoMockLocationProvider(true, false); } LocationProviderBase* NewAutoFailMockLocationProvider() { - return new AutoMockLocationProvider(false); + return new AutoMockLocationProvider(false, false); +} + +LocationProviderBase* NewAutoSuccessMockNetworkLocationProvider() { + return new AutoMockLocationProvider(true, true); } diff --git a/chrome/browser/geolocation/mock_location_provider.h b/chrome/browser/geolocation/mock_location_provider.h index b690416..50b454b 100644 --- a/chrome/browser/geolocation/mock_location_provider.h +++ b/chrome/browser/geolocation/mock_location_provider.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_GEOLOCATION_MOCK_LOCATION_PROVIDER_H_ #include "chrome/browser/geolocation/location_provider.h" +#include "googleurl/src/gurl.h" // Mock implementation of a location provider for testing. class MockLocationProvider : public LocationProviderBase { @@ -18,10 +19,12 @@ class MockLocationProvider : public LocationProviderBase { // LocationProviderBase implementation. virtual bool StartProvider(); - virtual void GetPosition(Geoposition *position); + virtual void GetPosition(Geoposition* position); + virtual void OnPermissionGranted(const GURL& requesting_frame); Geoposition position_; int started_count_; + GURL permission_granted_url_; // Set when an instance of the mock is created via a factory function. static MockLocationProvider* instance_; @@ -42,5 +45,9 @@ LocationProviderBase* NewAutoSuccessMockLocationProvider(); // Creates a mock location provider that automatically notifies its // listeners with an error when StartProvider is called. LocationProviderBase* NewAutoFailMockLocationProvider(); +// Similar to NewAutoSuccessMockLocationProvider but mimicks the behavior of +// the Network Location provider, in deferring making location updates until +// a permission request has been confirmed. +LocationProviderBase* NewAutoSuccessMockNetworkLocationProvider(); #endif // CHROME_BROWSER_GEOLOCATION_MOCK_LOCATION_PROVIDER_H_ diff --git a/chrome/browser/geolocation/network_location_provider.cc b/chrome/browser/geolocation/network_location_provider.cc index e737e88..82a7df7 100644 --- a/chrome/browser/geolocation/network_location_provider.cc +++ b/chrome/browser/geolocation/network_location_provider.cc @@ -102,10 +102,9 @@ LocationProviderBase* NewNetworkLocationProvider( AccessTokenStore* access_token_store, URLRequestContextGetter* context, const GURL& url, - const string16& access_token, - const string16& host_name) { + const string16& access_token) { return new NetworkLocationProvider( - access_token_store, context, url, access_token, host_name); + access_token_store, context, url, access_token); } // NetworkLocationProvider @@ -113,8 +112,7 @@ NetworkLocationProvider::NetworkLocationProvider( AccessTokenStore* access_token_store, URLRequestContextGetter* url_context_getter, const GURL& url, - const string16& access_token, - const string16& host_name) + const string16& access_token) : access_token_store_(access_token_store), radio_data_provider_(NULL), wifi_data_provider_(NULL), @@ -126,8 +124,7 @@ NetworkLocationProvider::NetworkLocationProvider( // Create the position cache. position_cache_.reset(new PositionCache()); - request_.reset(new NetworkLocationRequest(url_context_getter, url, - host_name, this)); + request_.reset(new NetworkLocationRequest(url_context_getter, url, this)); } NetworkLocationProvider::~NetworkLocationProvider() { @@ -144,11 +141,22 @@ void NetworkLocationProvider::GetPosition(Geoposition *position) { } void NetworkLocationProvider::UpdatePosition() { - if (is_new_data_available_) { + // Whilst in the delayed start, only send request if all data is ready. + if (delayed_start_task_.empty() || + (is_radio_data_complete_ && is_wifi_data_complete_)) { RequestPosition(); } } +void NetworkLocationProvider::OnPermissionGranted( + const GURL& requesting_frame) { + const bool host_was_empty = most_recent_authorized_host_.empty(); + most_recent_authorized_host_ = requesting_frame.host(); + if (host_was_empty && !most_recent_authorized_host_.empty()) { + UpdatePosition(); + } +} + // DeviceDataProviderInterface::ListenerInterface implementation. void NetworkLocationProvider::DeviceDataUpdateAvailable( RadioDataProvider* provider) { @@ -168,12 +176,14 @@ void NetworkLocationProvider::DeviceDataUpdateAvailable( void NetworkLocationProvider::LocationResponseAvailable( const Geoposition& position, bool server_error, - const string16& access_token) { + const string16& access_token, + const RadioData& radio_data, + const WifiData& wifi_data) { DCHECK(CalledOnValidThread()); // Record the position and update our cache. position_ = position; if (position.IsValidFix()) { - position_cache_->CachePosition(radio_data_, wifi_data_, position); + position_cache_->CachePosition(radio_data, wifi_data, position); } // Record access_token if it's set. @@ -217,10 +227,11 @@ bool NetworkLocationProvider::StartProvider() { // Other methods void NetworkLocationProvider::RequestPosition() { DCHECK(CalledOnValidThread()); + if (!is_new_data_available_) + return; - delayed_start_task_.RevokeAll(); - const Geoposition* cached_position; - cached_position = position_cache_->FindPosition(radio_data_, wifi_data_); + const Geoposition* cached_position = + position_cache_->FindPosition(radio_data_, wifi_data_); DCHECK(!device_data_updated_timestamp_.is_null()) << "Timestamp must be set before looking up position"; if (cached_position) { @@ -236,19 +247,25 @@ void NetworkLocationProvider::RequestPosition() { UpdateListeners(); return; } + // Don't send network requests until authorized. http://crbug.com/39171 + if (most_recent_authorized_host_.empty()) + return; + delayed_start_task_.RevokeAll(); is_new_data_available_ = false; // TODO(joth): Rather than cancel pending requests, we should create a new - // NetworkLocationRequest for each and hold a set of pending requests. This - // means the associated wifi data, timestamps etc. could be tagged onto each - // request allowing self-consistent cache update when each request completes. + // NetworkLocationRequest for each and hold a set of pending requests. if (request_->is_request_pending()) { LOG(INFO) << "NetworkLocationProvider - pre-empting pending network request" << "with new data. Wifi APs: " << wifi_data_.access_point_data.size(); } - request_->MakeRequest(access_token_, radio_data_, wifi_data_, + // The hostname sent in the request is just to give a first-order + // approximation of usage. We do not need to guarantee that this network + // request was triggered by an API call from this specific host. + request_->MakeRequest(most_recent_authorized_host_, access_token_, + radio_data_, wifi_data_, device_data_updated_timestamp_); } @@ -257,8 +274,5 @@ void NetworkLocationProvider::OnDeviceDataUpdated() { device_data_updated_timestamp_ = base::Time::Now(); is_new_data_available_ = is_radio_data_complete_ || is_wifi_data_complete_; - if (delayed_start_task_.empty() || - (is_radio_data_complete_ && is_wifi_data_complete_)) { - UpdatePosition(); - } + UpdatePosition(); } diff --git a/chrome/browser/geolocation/network_location_provider.h b/chrome/browser/geolocation/network_location_provider.h index 93072fa..c03d5c4 100644 --- a/chrome/browser/geolocation/network_location_provider.h +++ b/chrome/browser/geolocation/network_location_provider.h @@ -26,14 +26,14 @@ class NetworkLocationProvider NetworkLocationProvider(AccessTokenStore* access_token_store, URLRequestContextGetter* context, const GURL& url, - const string16& access_token, - const string16& host_name); + const string16& access_token); virtual ~NetworkLocationProvider(); // LocationProviderBase implementation virtual bool StartProvider(); virtual void GetPosition(Geoposition *position); virtual void UpdatePosition(); + virtual void OnPermissionGranted(const GURL& requesting_frame); private: // PositionCache is an implementation detail of NetworkLocationProvider. @@ -52,7 +52,9 @@ class NetworkLocationProvider // NetworkLocationRequest::ListenerInterface implementation. virtual void LocationResponseAvailable(const Geoposition& position, bool server_error, - const string16& access_token); + const string16& access_token, + const RadioData& radio_data, + const WifiData& wifi_data); scoped_refptr<AccessTokenStore> access_token_store_; @@ -78,6 +80,8 @@ class NetworkLocationProvider bool is_new_data_available_; + std::string most_recent_authorized_host_; + // The network location request object, and the url it uses. scoped_ptr<NetworkLocationRequest> request_; diff --git a/chrome/browser/geolocation/network_location_provider_unittest.cc b/chrome/browser/geolocation/network_location_provider_unittest.cc index 061eaf7..c27565e 100644 --- a/chrome/browser/geolocation/network_location_provider_unittest.cc +++ b/chrome/browser/geolocation/network_location_provider_unittest.cc @@ -16,6 +16,7 @@ namespace { const char kTestServerUrl[] = "https://www.geolocation.test/service"; const char kTestHost[] = "myclienthost.test"; +const char kTestHostUrl[] = "http://myclienthost.test/some/path"; } // namespace // Stops the specified (nested) message loop when the listener is called back. @@ -123,13 +124,15 @@ class GeolocationNetworkProviderTest : public testing::Test { base::LeakTracker<URLFetcher>::CheckForLeaks(); } - LocationProviderBase* CreateProvider() { - return NewNetworkLocationProvider( + LocationProviderBase* CreateProvider(bool set_permission_granted) { + LocationProviderBase* provider = NewNetworkLocationProvider( access_token_store_.get(), NULL, // No URLContextGetter needed, as using test urlfecther factory. test_server_url_, - access_token_store_->access_token_set_[test_server_url_], - ASCIIToUTF16(kTestHost)); + access_token_store_->access_token_set_[test_server_url_]); + if (set_permission_granted) + provider->OnPermissionGranted(GURL(kTestHostUrl)); + return provider; } protected: @@ -144,7 +147,7 @@ class GeolocationNetworkProviderTest : public testing::Test { // Returns the current url fetcher (if any) and advances the id ready for the // next test step. - TestURLFetcher* advance_url_fetcher_id() { + TestURLFetcher* get_url_fetcher_and_advance_id() { TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID( NetworkLocationRequest::url_fetcher_id_for_tests); if (fetcher) @@ -263,16 +266,16 @@ class GeolocationNetworkProviderTest : public testing::Test { TEST_F(GeolocationNetworkProviderTest, CreateDestroy) { // Test fixture members were SetUp correctly. EXPECT_EQ(&main_message_loop_, MessageLoop::current()); - scoped_ptr<LocationProviderBase> provider(CreateProvider()); + scoped_ptr<LocationProviderBase> provider(CreateProvider(true)); EXPECT_TRUE(NULL != provider.get()); provider.reset(); SUCCEED(); } TEST_F(GeolocationNetworkProviderTest, StartProvider) { - scoped_ptr<LocationProviderBase> provider(CreateProvider()); + scoped_ptr<LocationProviderBase> provider(CreateProvider(true)); EXPECT_TRUE(provider->StartProvider()); - TestURLFetcher* fetcher = advance_url_fetcher_id(); + TestURLFetcher* fetcher = get_url_fetcher_and_advance_id(); ASSERT_TRUE(fetcher != NULL); EXPECT_EQ(test_server_url_, fetcher->original_url()); @@ -284,7 +287,7 @@ TEST_F(GeolocationNetworkProviderTest, StartProvider) { TEST_F(GeolocationNetworkProviderTest, MultiRegistrations) { // TODO(joth): Strictly belongs in a base-class unit test file. MessageLoopQuitListener listener; - scoped_ptr<LocationProviderBase> provider(CreateProvider()); + scoped_ptr<LocationProviderBase> provider(CreateProvider(true)); EXPECT_FALSE(provider->has_listeners()); provider->RegisterListener(&listener); EXPECT_TRUE(provider->has_listeners()); @@ -298,10 +301,10 @@ TEST_F(GeolocationNetworkProviderTest, MultiRegistrations) { } TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { - scoped_ptr<LocationProviderBase> provider(CreateProvider()); + scoped_ptr<LocationProviderBase> provider(CreateProvider(true)); EXPECT_TRUE(provider->StartProvider()); - TestURLFetcher* fetcher = advance_url_fetcher_id(); + TestURLFetcher* fetcher = get_url_fetcher_and_advance_id(); ASSERT_TRUE(fetcher != NULL); CheckEmptyRequestIsValid(fetcher->upload_data()); // Complete the network request with bad position fix (using #define so we @@ -328,7 +331,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { const int kFirstScanAps = 6; wifi_data_provider_->SetData(CreateReferenceWifiScanData(kFirstScanAps)); main_message_loop_.RunAllPending(); - fetcher = advance_url_fetcher_id(); + fetcher = get_url_fetcher_and_advance_id(); ASSERT_TRUE(fetcher != NULL); // The request should have access token (set previously) and the wifi data. CheckRequestIsValid(fetcher->upload_data(), @@ -368,7 +371,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { const int kSecondScanAps = kFirstScanAps - 1; wifi_data_provider_->SetData(CreateReferenceWifiScanData(kSecondScanAps)); main_message_loop_.RunAllPending(); - fetcher = advance_url_fetcher_id(); + fetcher = get_url_fetcher_and_advance_id(); EXPECT_FALSE(fetcher); provider->GetPosition(&position); @@ -380,7 +383,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { const int kThirdScanAps = kFirstScanAps * 2 + 1; wifi_data_provider_->SetData(CreateReferenceWifiScanData(kThirdScanAps)); main_message_loop_.RunAllPending(); - fetcher = advance_url_fetcher_id(); + fetcher = get_url_fetcher_and_advance_id(); EXPECT_TRUE(fetcher); // ...reply with a network error. fetcher->delegate()->OnURLFetchComplete( @@ -397,7 +400,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { // Wifi scan returns to original set: should be serviced from cache. wifi_data_provider_->SetData(CreateReferenceWifiScanData(kFirstScanAps)); main_message_loop_.RunAllPending(); - EXPECT_FALSE(advance_url_fetcher_id()); // No new request created. + EXPECT_FALSE(get_url_fetcher_and_advance_id()); // No new request created. provider->GetPosition(&position); EXPECT_EQ(51.0, position.latitude); @@ -408,30 +411,73 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { TEST_F(GeolocationNetworkProviderTest, NoRequestOnStartupUntilWifiData) { MessageLoopQuitListener listener; wifi_data_provider_->set_got_data(false); - scoped_ptr<LocationProviderBase> provider(CreateProvider()); + scoped_ptr<LocationProviderBase> provider(CreateProvider(true)); EXPECT_TRUE(provider->StartProvider()); provider->RegisterListener(&listener); main_message_loop_.RunAllPending(); - EXPECT_FALSE(advance_url_fetcher_id()) + EXPECT_FALSE(get_url_fetcher_and_advance_id()) << "Network request should not be created right away on startup when " "wifi data has not yet arrived"; wifi_data_provider_->SetData(CreateReferenceWifiScanData(1)); main_message_loop_.RunAllPending(); - EXPECT_TRUE(advance_url_fetcher_id()); + EXPECT_TRUE(get_url_fetcher_and_advance_id()); } TEST_F(GeolocationNetworkProviderTest, NewDataReplacesExistingNetworkRequest) { // Send initial request with empty device data - scoped_ptr<LocationProviderBase> provider(CreateProvider()); + scoped_ptr<LocationProviderBase> provider(CreateProvider(true)); EXPECT_TRUE(provider->StartProvider()); - TestURLFetcher* fetcher = advance_url_fetcher_id(); + TestURLFetcher* fetcher = get_url_fetcher_and_advance_id(); EXPECT_TRUE(fetcher); // Now wifi data arrives; new request should be sent. wifi_data_provider_->SetData(CreateReferenceWifiScanData(4)); main_message_loop_.RunAllPending(); - fetcher = advance_url_fetcher_id(); + fetcher = get_url_fetcher_and_advance_id(); EXPECT_TRUE(fetcher); } + +TEST_F(GeolocationNetworkProviderTest, NetworkRequestDeferredForPermission) { + scoped_ptr<LocationProviderBase> provider(CreateProvider(false)); + EXPECT_TRUE(provider->StartProvider()); + TestURLFetcher* fetcher = get_url_fetcher_and_advance_id(); + EXPECT_FALSE(fetcher); + provider->OnPermissionGranted(GURL(kTestHostUrl)); + + fetcher = get_url_fetcher_and_advance_id(); + ASSERT_TRUE(fetcher != NULL); + + EXPECT_EQ(test_server_url_, fetcher->original_url()); + + // No wifi data so expect an empty request. + CheckEmptyRequestIsValid(fetcher->upload_data()); +} + +TEST_F(GeolocationNetworkProviderTest, + NetworkRequestWithWifiDataDeferredForPermission) { + access_token_store_->access_token_set_[test_server_url_] = + UTF8ToUTF16(REFERENCE_ACCESS_TOKEN); + scoped_ptr<LocationProviderBase> provider(CreateProvider(false)); + EXPECT_TRUE(provider->StartProvider()); + TestURLFetcher* fetcher = get_url_fetcher_and_advance_id(); + EXPECT_FALSE(fetcher); + + static const int kScanCount = 4; + wifi_data_provider_->SetData(CreateReferenceWifiScanData(kScanCount)); + main_message_loop_.RunAllPending(); + + fetcher = get_url_fetcher_and_advance_id(); + EXPECT_FALSE(fetcher); + + provider->OnPermissionGranted(GURL(kTestHostUrl)); + + fetcher = get_url_fetcher_and_advance_id(); + ASSERT_TRUE(fetcher != NULL); + + EXPECT_EQ(test_server_url_, fetcher->original_url()); + + CheckRequestIsValid(fetcher->upload_data(), kScanCount, + REFERENCE_ACCESS_TOKEN); +} diff --git a/chrome/browser/geolocation/network_location_request.cc b/chrome/browser/geolocation/network_location_request.cc index a2961c9..01bfd93 100644 --- a/chrome/browser/geolocation/network_location_request.cc +++ b/chrome/browser/geolocation/network_location_request.cc @@ -14,22 +14,22 @@ #include "net/url_request/url_request_status.h" namespace { -const char* const kMimeApplicationJson = "application/json"; +const char kMimeApplicationJson[] = "application/json"; // See http://code.google.com/apis/gears/geolocation_network_protocol.html -const char* kGeoLocationNetworkProtocolVersion = "1.1.0"; +const char kGeoLocationNetworkProtocolVersion[] = "1.1.0"; -const wchar_t* kAccessTokenString = L"access_token"; -const wchar_t* kLocationString = L"location"; -const wchar_t* kLatitudeString = L"latitude"; -const wchar_t* kLongitudeString = L"longitude"; -const wchar_t* kAltitudeString = L"altitude"; -const wchar_t* kAccuracyString = L"accuracy"; -const wchar_t* kAltitudeAccuracyString = L"altitude_accuracy"; +const wchar_t kAccessTokenString[] = L"access_token"; +const wchar_t kLocationString[] = L"location"; +const wchar_t kLatitudeString[] = L"latitude"; +const wchar_t kLongitudeString[] = L"longitude"; +const wchar_t kAltitudeString[] = L"altitude"; +const wchar_t kAccuracyString[] = L"accuracy"; +const wchar_t kAltitudeAccuracyString[] = L"altitude_accuracy"; // Local functions // Creates the request payload to send to the server. -bool FormRequestBody(const string16& host_name, +void FormRequestBody(const std::string& host_name, const string16& access_token, const RadioData& radio_data, const WifiData& wifi_data, @@ -70,17 +70,17 @@ int NetworkLocationRequest::url_fetcher_id_for_tests = 0; NetworkLocationRequest::NetworkLocationRequest(URLRequestContextGetter* context, const GURL& url, - const string16& host_name, ListenerInterface* listener) : url_context_(context), listener_(listener), - url_(url), host_name_(host_name) { + url_(url) { DCHECK(listener); } NetworkLocationRequest::~NetworkLocationRequest() { } -bool NetworkLocationRequest::MakeRequest(const string16& access_token, +bool NetworkLocationRequest::MakeRequest(const std::string& host_name, + const string16& access_token, const RadioData& radio_data, const WifiData& wifi_data, const base::Time& timestamp) { @@ -88,12 +88,12 @@ bool NetworkLocationRequest::MakeRequest(const string16& access_token, DLOG(INFO) << "NetworkLocationRequest : Cancelling pending request"; url_fetcher_.reset(); } - std::string post_body; - if (!FormRequestBody(host_name_, access_token, radio_data, wifi_data, - timestamp, &post_body)) { - return false; - } + radio_data_ = radio_data; + wifi_data_ = wifi_data; timestamp_ = timestamp; + std::string post_body; + FormRequestBody(host_name, access_token, radio_data_, wifi_data_, + timestamp_, &post_body); url_fetcher_.reset(URLFetcher::Create( url_fetcher_id_for_tests, url_, URLFetcher::POST, this)); @@ -127,13 +127,14 @@ void NetworkLocationRequest::OnURLFetchComplete(const URLFetcher* source, DCHECK(listener_); DLOG(INFO) << "NetworkLocationRequest::Run() : " "Calling listener with position.\n"; - listener_->LocationResponseAvailable(position, server_error, access_token); + listener_->LocationResponseAvailable(position, server_error, access_token, + radio_data_, wifi_data_); } // Local functions. namespace { -bool FormRequestBody(const string16& host_name, +void FormRequestBody(const std::string& host_name, const string16& access_token, const RadioData& radio_data, const WifiData& wifi_data, @@ -143,11 +144,11 @@ bool FormRequestBody(const string16& host_name, DictionaryValue body_object; // Version and host are required. - if (host_name.empty()) { - return false; - } + COMPILE_ASSERT(sizeof(kGeoLocationNetworkProtocolVersion) > 1, + must_include_valid_version); + DCHECK(!host_name.empty()); body_object.SetString(L"version", kGeoLocationNetworkProtocolVersion); - AddString(L"host", host_name, &body_object); + body_object.SetString(L"host", host_name); AddString(L"access_token", access_token, &body_object); @@ -166,7 +167,6 @@ bool FormRequestBody(const string16& host_name, base::JSONWriter::Write(&body_object, false, data); DLOG(INFO) << "NetworkLocationRequest::FormRequestBody(): Formed body " << *data << ".\n"; - return true; } void FormatPositionError(const GURL& server_url, diff --git a/chrome/browser/geolocation/network_location_request.h b/chrome/browser/geolocation/network_location_request.h index 7f1b399..e08d570 100644 --- a/chrome/browser/geolocation/network_location_request.h +++ b/chrome/browser/geolocation/network_location_request.h @@ -31,24 +31,24 @@ class NetworkLocationRequest : private URLFetcher::Delegate { virtual void LocationResponseAvailable( const Geoposition& position, bool server_error, - const string16& access_token) = 0; + const string16& access_token, + const RadioData& radio_data, + const WifiData& wifi_data) = 0; protected: virtual ~ListenerInterface() {} }; - // |url| is the server address to which the request wil be sent, |host_name| - // is the host of the webpage that caused this request. - // TODO(joth): is host needed? What to do when we reuse cached locations? + // |url| is the server address to which the request wil be sent. NetworkLocationRequest(URLRequestContextGetter* context, const GURL& url, - const string16& host_name, ListenerInterface* listener); virtual ~NetworkLocationRequest(); // Makes a new request. Returns true if the new request was successfully // started. In all cases, any currently pending request will be canceled. - bool MakeRequest(const string16& access_token, + bool MakeRequest(const std::string& host, + const string16& access_token, const RadioData& radio_data, const WifiData& wifi_data, const base::Time& timestamp); @@ -66,12 +66,16 @@ class NetworkLocationRequest : private URLFetcher::Delegate { const std::string& data); scoped_refptr<URLRequestContextGetter> url_context_; - base::Time timestamp_; // The timestamp of the data used to make the request. ListenerInterface* listener_; const GURL url_; - string16 host_name_; scoped_ptr<URLFetcher> url_fetcher_; + // Keep a copy of the data sent in the request, so we can refer back to it + // when the response arrives. + RadioData radio_data_; + WifiData wifi_data_; + base::Time timestamp_; // Timestamp of the above data, not of the request. + DISALLOW_EVIL_CONSTRUCTORS(NetworkLocationRequest); }; |