diff options
11 files changed, 235 insertions, 281 deletions
diff --git a/chrome/browser/geolocation/access_token_store.cc b/chrome/browser/geolocation/access_token_store.cc index d037887..b0d2d5e 100644 --- a/chrome/browser/geolocation/access_token_store.cc +++ b/chrome/browser/geolocation/access_token_store.cc @@ -15,144 +15,95 @@ namespace { class ChromePrefsAccessTokenStore : public AccessTokenStore { public: - ChromePrefsAccessTokenStore( - const GURL& url, bool token_valid, const string16& initial_access_token); + ChromePrefsAccessTokenStore(); private: - // AccessTokenStore - virtual void DoSetAccessToken(const string16& access_token); -}; - - -class ChromePrefsAccessTokenStoreFactory : public AccessTokenStoreFactory { - private: - void LoadDictionaryStoreInUIThread(ChromeThread::ID client_thread_id, - const GURL& default_url); - void NotifyDelegateInClientThread(const TokenStoreSet& token_stores); + void LoadDictionaryStoreInUIThread( + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request); - // AccessTokenStoreFactory - virtual void CreateAccessTokenStores( - const base::WeakPtr<AccessTokenStoreFactory::Delegate>& delegate, - const GURL& default_url); + // AccessTokenStore + virtual void DoLoadAccessTokens( + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request); + virtual void SaveAccessToken( + const GURL& server_url, const string16& access_token); - base::WeakPtr<AccessTokenStoreFactory::Delegate> delegate_; + DISALLOW_COPY_AND_ASSIGN(ChromePrefsAccessTokenStore); }; -ChromePrefsAccessTokenStore::ChromePrefsAccessTokenStore( - const GURL& url, bool token_valid, const string16& initial_access_token) - : AccessTokenStore(url, token_valid, initial_access_token) { +ChromePrefsAccessTokenStore::ChromePrefsAccessTokenStore() { } -void ChromePrefsAccessTokenStore::DoSetAccessToken( - const string16& access_token) { - if (!ChromeThread::CurrentlyOn(ChromeThread::UI)) { - ChromeThread::PostTask( - ChromeThread::UI, FROM_HERE, NewRunnableMethod( - this, &ChromePrefsAccessTokenStore::DoSetAccessToken, - access_token)); - return; - } - DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); - DictionaryValue* access_token_dictionary = - g_browser_process->local_state()->GetMutableDictionary( - prefs::kGeolocationAccessToken); - access_token_dictionary->SetWithoutPathExpansion( - UTF8ToWide(server_url().spec()), - Value::CreateStringValueFromUTF16(access_token)); -} - -void ChromePrefsAccessTokenStoreFactory::LoadDictionaryStoreInUIThread( - ChromeThread::ID client_thread_id, const GURL& default_url) { +void ChromePrefsAccessTokenStore::LoadDictionaryStoreInUIThread( + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + if (request->canceled()) + return; const DictionaryValue* token_dictionary = - g_browser_process->local_state()->GetDictionary( - prefs::kGeolocationAccessToken); - TokenStoreSet token_stores; - // Careful: The returned value could be NULL if the pref has never been set. + g_browser_process->local_state()->GetDictionary( + prefs::kGeolocationAccessToken); + + AccessTokenStore::AccessTokenSet access_token_set; + // The dictionary value could be NULL if the pref has never been set. if (token_dictionary != NULL) { for (DictionaryValue::key_iterator it = token_dictionary->begin_keys(); it != token_dictionary->end_keys(); ++it) { - GURL this_url(WideToUTF8(*it)); - if (!this_url.is_valid()) + GURL url(WideToUTF8(*it)); + if (!url.is_valid()) continue; - string16 token; - const bool token_valid = - token_dictionary->GetStringAsUTF16WithoutPathExpansion(*it, &token); - token_stores[this_url] = - new ChromePrefsAccessTokenStore(this_url, token_valid, token); + token_dictionary->GetStringAsUTF16WithoutPathExpansion( + *it, &access_token_set[url]); } } - if (default_url.is_valid() && token_stores[default_url] == NULL) { - token_stores[default_url] = - new ChromePrefsAccessTokenStore(default_url, false, string16()); - } - ChromeThread::PostTask( - client_thread_id, FROM_HERE, NewRunnableMethod( - this, - &ChromePrefsAccessTokenStoreFactory::NotifyDelegateInClientThread, - token_stores)); + request->ForwardResultAsync(MakeTuple(access_token_set)); } -void ChromePrefsAccessTokenStoreFactory::NotifyDelegateInClientThread( - const TokenStoreSet& token_stores) { - if (delegate_ != NULL) { - delegate_->OnAccessTokenStoresCreated(token_stores); - } +void ChromePrefsAccessTokenStore::DoLoadAccessTokens( + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) { + ChromeThread::PostTask(ChromeThread::UI, FROM_HERE, NewRunnableMethod( + this, &ChromePrefsAccessTokenStore::LoadDictionaryStoreInUIThread, + request)); } -void ChromePrefsAccessTokenStoreFactory::CreateAccessTokenStores( - const base::WeakPtr<AccessTokenStoreFactory::Delegate>& delegate, - const GURL& default_url) { - // Capture the thread that created the factory, in order to make callbacks - // on the same thread. We could capture a MessageLoop* but in practice we only - // expect to be called from well-known chrome threads, so this is sufficient. - ChromeThread::ID client_thread_id; - bool ok = ChromeThread::GetCurrentThreadIdentifier(&client_thread_id); - CHECK(ok); - DCHECK_NE(ChromeThread::UI, client_thread_id) - << "If I'm only used from the UI thread I don't need to post tasks"; - delegate_ = delegate; - ChromeThread::PostTask( - ChromeThread::UI, FROM_HERE, NewRunnableMethod( - this, - &ChromePrefsAccessTokenStoreFactory::LoadDictionaryStoreInUIThread, - client_thread_id, default_url)); +void SetAccessTokenOnUIThread(const GURL& server_url, const string16& token) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + DictionaryValue* access_token_dictionary = + g_browser_process->local_state()->GetMutableDictionary( + prefs::kGeolocationAccessToken); + access_token_dictionary->SetWithoutPathExpansion( + UTF8ToWide(server_url.spec()), + Value::CreateStringValueFromUTF16(token)); } -} // namespace -void AccessTokenStore::RegisterPrefs(PrefService* prefs) { - prefs->RegisterDictionaryPref(prefs::kGeolocationAccessToken); +void ChromePrefsAccessTokenStore::SaveAccessToken( + const GURL& server_url, const string16& access_token) { + ChromeThread::PostTask(ChromeThread::UI, FROM_HERE, NewRunnableFunction( + &SetAccessTokenOnUIThread, server_url, access_token)); } +} // namespace -AccessTokenStore::AccessTokenStore( - const GURL& url, bool token_valid, const string16& initial_access_token) - : url_(url), access_token_valid_(token_valid), - access_token_(initial_access_token) { +AccessTokenStore::AccessTokenStore() { } AccessTokenStore::~AccessTokenStore() { } -GURL AccessTokenStore::server_url() const { - return url_; -} - -void AccessTokenStore::SetAccessToken(const string16& access_token) { - access_token_ = access_token; - access_token_valid_ = true; - DoSetAccessToken(access_token); +void AccessTokenStore::RegisterPrefs(PrefService* prefs) { + prefs->RegisterDictionaryPref(prefs::kGeolocationAccessToken); } -bool AccessTokenStore::GetAccessToken(string16* access_token) const { - DCHECK(access_token); - *access_token = access_token_; - return access_token_valid_; -} +AccessTokenStore::Handle AccessTokenStore::LoadAccessTokens( + CancelableRequestConsumerBase* consumer, + LoadAccessTokensCallbackType* callback) { + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request = + new CancelableRequest<LoadAccessTokensCallbackType>(callback); + AddRequest(request, consumer); + DCHECK(request->handle()); -AccessTokenStoreFactory::~AccessTokenStoreFactory() { + DoLoadAccessTokens(request); + return request->handle(); } // Creates a new access token store backed by the global chome prefs. -AccessTokenStoreFactory* NewChromePrefsAccessTokenStoreFactory() { - return new ChromePrefsAccessTokenStoreFactory; +AccessTokenStore* NewChromePrefsAccessTokenStore() { + return new ChromePrefsAccessTokenStore; } diff --git a/chrome/browser/geolocation/access_token_store.h b/chrome/browser/geolocation/access_token_store.h index 98a51fa..8ecc81c 100644 --- a/chrome/browser/geolocation/access_token_store.h +++ b/chrome/browser/geolocation/access_token_store.h @@ -2,13 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Defines the Geolocation access token store, and associated factory. -// An access token store is responsible for providing the API to persist a -// single network provider's access token. The factory is responsible for -// first loading tokens when required, and creating the associated token stores. +// Defines the Geolocation access token store, and associated factory function. +// An access token store is responsible for providing the API to persist +// access tokens, one at a time, and to load them back on mass. // The API is a little more complex than one might wish, due to the need for // prefs access to happen asynchronously on the UI thread. -// These APIs are provided as abstract base classes to allow mocking and testing +// This API is provided as abstract base classes to allow mocking and testing // of clients, without dependency on browser process singleton objects etc. #ifndef CHROME_BROWSER_GEOLOCATION_ACCESS_TOKEN_STORE_H_ @@ -18,63 +17,44 @@ #include "base/ref_counted.h" #include "base/string16.h" -#include "base/weak_ptr.h" +#include "base/task.h" +#include "chrome/browser/cancelable_request.h" #include "googleurl/src/gurl.h" class GURL; class PrefService; // Provides storage for the access token used in the network request. -class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore> { +class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore>, + public CancelableRequestProvider { public: static void RegisterPrefs(PrefService* prefs); - GURL server_url() const; - void SetAccessToken(const string16& access_token); - bool GetAccessToken(string16* access_token) const; + // Map of server URLs to associated access token. + typedef std::map<GURL, string16> AccessTokenSet; + typedef Callback1<AccessTokenSet>::Type LoadAccessTokensCallbackType; + // callback will be invoked once, after existing access tokens have + // been loaded from persistent store. Takes ownership of callback. + // Returns a handle which can subsequently be used with CancelRequest(). + Handle LoadAccessTokens(CancelableRequestConsumerBase* consumer, + LoadAccessTokensCallbackType* callback); + + virtual void SaveAccessToken( + const GURL& server_url, const string16& access_token) = 0; protected: friend class base::RefCountedThreadSafe<AccessTokenStore>; - AccessTokenStore( - const GURL& url, bool token_valid, const string16& initial_access_token); + AccessTokenStore(); virtual ~AccessTokenStore(); - virtual void DoSetAccessToken(const string16& access_token) = 0; + virtual void DoLoadAccessTokens( + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > req) = 0; private: - const GURL url_; - bool access_token_valid_; - string16 access_token_; -}; - -// Factory for access token stores. Asynchronously loads all access tokens, and -// calls back with a set of token stores one per server URL. -class AccessTokenStoreFactory - : public base::RefCountedThreadSafe<AccessTokenStoreFactory> { - public: - typedef std::map<GURL, scoped_refptr<AccessTokenStore> > TokenStoreSet; - class Delegate { - public: - virtual void OnAccessTokenStoresCreated( - const TokenStoreSet& access_token_store) = 0; - - protected: - virtual ~Delegate() {} - }; - // delegate will recieve a single callback once existing access tokens have - // been loaded from persistent store. - // If default_url is valid, this additional URL will be added to the - // set of access token stores returned. - virtual void CreateAccessTokenStores( - const base::WeakPtr<AccessTokenStoreFactory::Delegate>& delegate, - const GURL& default_url) = 0; - - protected: - friend class base::RefCountedThreadSafe<AccessTokenStoreFactory>; - virtual ~AccessTokenStoreFactory(); + DISALLOW_COPY_AND_ASSIGN(AccessTokenStore); }; // Creates a new access token store backed by the global chome prefs. -AccessTokenStoreFactory* NewChromePrefsAccessTokenStoreFactory(); +AccessTokenStore* NewChromePrefsAccessTokenStore(); #endif // CHROME_BROWSER_GEOLOCATION_ACCESS_TOKEN_STORE_H_ diff --git a/chrome/browser/geolocation/access_token_store_browsertest.cc b/chrome/browser/geolocation/access_token_store_browsertest.cc index 63d1d12..062fb28 100644 --- a/chrome/browser/geolocation/access_token_store_browsertest.cc +++ b/chrome/browser/geolocation/access_token_store_browsertest.cc @@ -18,41 +18,73 @@ const char* kRefServerUrl2 = "http://another.domain.example/foo?id=bar.bar#2"; } // namespace class GeolocationAccessTokenStoreTest - : public InProcessBrowserTest, - public AccessTokenStoreFactory::Delegate, - public base::SupportsWeakPtr<GeolocationAccessTokenStoreTest> { - protected: + : public InProcessBrowserTest { + public: GeolocationAccessTokenStoreTest() : token_to_expect_(NULL), token_to_set_(NULL) {} - void StartThreadAndWaitForResults( + void DoTestStepAndWaitForResults( const char* ref_url, const string16* token_to_expect, const string16* token_to_set); - // AccessTokenStoreFactory::Delegate - virtual void OnAccessTokenStoresCreated( - const AccessTokenStoreFactory::TokenStoreSet& access_token_store); + void OnAccessTokenStoresLoaded( + AccessTokenStore::AccessTokenSet access_token_set); + + scoped_refptr<AccessTokenStore> token_store_; + CancelableRequestConsumer request_consumer_; GURL ref_url_; const string16* token_to_expect_; const string16* token_to_set_; }; -namespace { -// A WeakPtr may only be used on the thread in which it is created, hence we -// defer the call to delegate->AsWeakPtr() into this function rather than pass -// WeakPtr& in. -void StartTestFromClientThread( - GeolocationAccessTokenStoreTest* delegate, - const GURL& ref_url) { +void StartTestStepFromClientThread( + scoped_refptr<AccessTokenStore>* store, + CancelableRequestConsumerBase* consumer, + AccessTokenStore::LoadAccessTokensCallbackType* callback) { ASSERT_TRUE(ChromeThread::CurrentlyOn(kExpectedClientThreadId)); + if (*store == NULL) + (*store) = NewChromePrefsAccessTokenStore(); + (*store)->LoadAccessTokens(consumer, callback); +} - scoped_refptr<AccessTokenStoreFactory> store = - NewChromePrefsAccessTokenStoreFactory(); - store->CreateAccessTokenStores(delegate->AsWeakPtr(), ref_url); +struct TokenLoadClientForTest { + void NotReachedCallback(AccessTokenStore::AccessTokenSet /*tokens*/) { + NOTREACHED() << "This request should have been canceled before callback"; + } +}; + +void RunCancelTestInClientTread() { + ASSERT_TRUE(ChromeThread::CurrentlyOn(kExpectedClientThreadId)); + scoped_refptr<AccessTokenStore> store = NewChromePrefsAccessTokenStore(); + CancelableRequestConsumer consumer; + TokenLoadClientForTest load_client; + + // Single request, canceled explicitly + CancelableRequestProvider::Handle first_handle = + store->LoadAccessTokens(&consumer, NewCallback( + &load_client, &TokenLoadClientForTest::NotReachedCallback)); + EXPECT_TRUE(consumer.HasPendingRequests()); + // Test this handle is valid. + consumer.GetClientData(store.get(), first_handle); + store->CancelRequest(first_handle); + EXPECT_FALSE(consumer.HasPendingRequests()); + + // 2 requests, canceled globally. + store->LoadAccessTokens(&consumer, NewCallback( + &load_client, &TokenLoadClientForTest::NotReachedCallback)); + store->LoadAccessTokens(&consumer, NewCallback( + &load_client, &TokenLoadClientForTest::NotReachedCallback)); + EXPECT_TRUE(consumer.HasPendingRequests()); + EXPECT_EQ(2u, consumer.PendingRequestCount()); + consumer.CancelAllRequests(); + EXPECT_FALSE(consumer.HasPendingRequests()); + EXPECT_EQ(0u, consumer.PendingRequestCount()); + + ChromeThread::PostTask( + ChromeThread::UI, FROM_HERE, new MessageLoop::QuitTask); } -} // namespace -void GeolocationAccessTokenStoreTest::StartThreadAndWaitForResults( +void GeolocationAccessTokenStoreTest::DoTestStepAndWaitForResults( const char* ref_url, const string16* token_to_expect, const string16* token_to_set) { ref_url_ = GURL(ref_url); @@ -61,61 +93,57 @@ void GeolocationAccessTokenStoreTest::StartThreadAndWaitForResults( ChromeThread::PostTask( kExpectedClientThreadId, FROM_HERE, NewRunnableFunction( - &StartTestFromClientThread, this, ref_url_)); + &StartTestStepFromClientThread, &token_store_, &request_consumer_, + NewCallback(this, + &GeolocationAccessTokenStoreTest::OnAccessTokenStoresLoaded))); ui_test_utils::RunMessageLoop(); } -void GeolocationAccessTokenStoreTest::OnAccessTokenStoresCreated( - const AccessTokenStoreFactory::TokenStoreSet& access_token_store) { +void GeolocationAccessTokenStoreTest::OnAccessTokenStoresLoaded( + AccessTokenStore::AccessTokenSet access_token_set) { ASSERT_TRUE(ChromeThread::CurrentlyOn(kExpectedClientThreadId)) << "Callback from token factory should be from the same thread as the " - "CreateAccessTokenStores request was made on"; + "LoadAccessTokenStores request was made on"; EXPECT_TRUE(token_to_set_ || token_to_expect_) << "No work to do?"; - DCHECK_GE(access_token_store.size(), size_t(1)); - - AccessTokenStoreFactory::TokenStoreSet::const_iterator item = - access_token_store.find(ref_url_); - ASSERT_TRUE(item != access_token_store.end()); - scoped_refptr<AccessTokenStore> store = item->second; - ASSERT_TRUE(NULL != store); - string16 token; - bool read_ok = store->GetAccessToken(&token); + AccessTokenStore::AccessTokenSet::const_iterator item = + access_token_set.find(ref_url_); if (!token_to_expect_) { - EXPECT_FALSE(read_ok); - EXPECT_TRUE(token.empty()); + EXPECT_TRUE(item == access_token_set.end()); } else { - ASSERT_TRUE(read_ok); - EXPECT_EQ(*token_to_expect_, token); + EXPECT_FALSE(item == access_token_set.end()); + EXPECT_EQ(*token_to_expect_, item->second); } if (token_to_set_) { - store->SetAccessToken(*token_to_set_); + scoped_refptr<AccessTokenStore> store = + NewChromePrefsAccessTokenStore(); + store->SaveAccessToken(ref_url_, *token_to_set_); } ChromeThread::PostTask( ChromeThread::UI, FROM_HERE, new MessageLoop::QuitTask); } -#if !defined(OS_WIN) -// TODO(joth): Crashes on Linux and Mac. See http://crbug.com/36068. -#define MAYBE_SetAcrossInstances DISABLED_SetAcrossInstances -#else -#define MAYBE_SetAcrossInstances SetAcrossInstances -#endif - -IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, MAYBE_SetAcrossInstances) { +IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, SetAcrossInstances) { const string16 ref_token1 = ASCIIToUTF16("jksdfo90,'s#\"#1*("); const string16 ref_token2 = ASCIIToUTF16("\1\2\3\4\5\6\7\10\11\12=023"); ASSERT_TRUE(ChromeThread::CurrentlyOn(ChromeThread::UI)); - StartThreadAndWaitForResults(kRefServerUrl1, NULL, &ref_token1); + DoTestStepAndWaitForResults(kRefServerUrl1, NULL, &ref_token1); // Check it was set, and change to new value. - StartThreadAndWaitForResults(kRefServerUrl1, &ref_token1, &ref_token2); + DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, &ref_token2); // And change back. - StartThreadAndWaitForResults(kRefServerUrl1, &ref_token2, &ref_token1); - StartThreadAndWaitForResults(kRefServerUrl1, &ref_token1, NULL); + DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token2, &ref_token1); + DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, NULL); // Set a second server URL - StartThreadAndWaitForResults(kRefServerUrl2, NULL, &ref_token2); - StartThreadAndWaitForResults(kRefServerUrl2, &ref_token2, NULL); - StartThreadAndWaitForResults(kRefServerUrl1, &ref_token1, NULL); + DoTestStepAndWaitForResults(kRefServerUrl2, NULL, &ref_token2); + DoTestStepAndWaitForResults(kRefServerUrl2, &ref_token2, NULL); + DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, NULL); +} + +IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, CancelRequest) { + ChromeThread::PostTask( + kExpectedClientThreadId, FROM_HERE, NewRunnableFunction( + RunCancelTestInClientTread)); + ui_test_utils::RunMessageLoop(); } diff --git a/chrome/browser/geolocation/fake_access_token_store.h b/chrome/browser/geolocation/fake_access_token_store.h index 7e0f875..d9d272c 100644 --- a/chrome/browser/geolocation/fake_access_token_store.h +++ b/chrome/browser/geolocation/fake_access_token_store.h @@ -11,38 +11,34 @@ // A fake (non-persisted) access token store instance useful for testing. class FakeAccessTokenStore : public AccessTokenStore { public: - explicit FakeAccessTokenStore(const GURL& url) - : AccessTokenStore(url, false, string16()) {} + FakeAccessTokenStore() {} - virtual void DoSetAccessToken(const string16& access_token) { - access_token_set_ = access_token; + void NotifyDelegateTokensLoaded() { + CHECK(request_ != NULL); + request_->ForwardResult(MakeTuple(access_token_set_)); + request_ = NULL; } - string16 access_token_set_; + // AccessTokenStore + virtual void DoLoadAccessTokens( + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) { + DCHECK(request_ == NULL) + << "Fake token store currently only allows one request at a time"; + request_ = request; + } + virtual void SaveAccessToken( + const GURL& server_url, const string16& access_token) { + DCHECK(server_url.is_valid()); + access_token_set_[server_url] = access_token; + } + + AccessTokenSet access_token_set_; + scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request_; private: virtual ~FakeAccessTokenStore() {} -}; - -class FakeAccessTokenStoreFactory : public AccessTokenStoreFactory { - public: - void NotifyDelegateStoreCreated() { - ASSERT_TRUE(delegate_ != NULL); - AccessTokenStoreFactory::TokenStoreSet token_stores; - token_stores[default_url_] = new FakeAccessTokenStore(default_url_); - delegate_->OnAccessTokenStoresCreated(token_stores); - } - - // AccessTokenStoreFactory - virtual void CreateAccessTokenStores( - const base::WeakPtr<AccessTokenStoreFactory::Delegate>& delegate, - const GURL& default_url) { - delegate_ = delegate; - default_url_ = default_url; - } - base::WeakPtr<AccessTokenStoreFactory::Delegate> delegate_; - GURL default_url_; + DISALLOW_COPY_AND_ASSIGN(FakeAccessTokenStore); }; #endif // CHROME_BROWSER_GEOLOCATION_FAKE_ACCESS_TOKEN_STORE_H_ diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc index c82fb1d..6176239 100644 --- a/chrome/browser/geolocation/location_arbitrator.cc +++ b/chrome/browser/geolocation/location_arbitrator.cc @@ -24,10 +24,9 @@ const char* kDefaultNetworkProviderUrl = "https://www.google.com/loc/json"; class GeolocationArbitratorImpl : public GeolocationArbitrator, public LocationProviderBase::ListenerInterface, - public AccessTokenStoreFactory::Delegate, public NonThreadSafe { public: - GeolocationArbitratorImpl(AccessTokenStoreFactory* access_token_store_factory, + GeolocationArbitratorImpl(AccessTokenStore* access_token_store, URLRequestContextGetter* context_getter); virtual ~GeolocationArbitratorImpl(); @@ -41,15 +40,14 @@ class GeolocationArbitratorImpl virtual void LocationUpdateAvailable(LocationProviderBase* provider); virtual void MovementDetected(LocationProviderBase* provider); - // AccessTokenStoreFactory::Delegate - virtual void OnAccessTokenStoresCreated( - const AccessTokenStoreFactory::TokenStoreSet& access_token_store); + void OnAccessTokenStoresLoaded( + AccessTokenStore::AccessTokenSet access_token_store); private: void CreateProviders(); bool HasHighAccuracyObserver(); - scoped_refptr<AccessTokenStoreFactory> access_token_store_factory_; + scoped_refptr<AccessTokenStore> access_token_store_; scoped_refptr<URLRequestContextGetter> context_getter_; const GURL default_url_; @@ -58,18 +56,16 @@ class GeolocationArbitratorImpl typedef std::map<GeolocationArbitrator::Delegate*, UpdateOptions> DelegateMap; DelegateMap observers_; - base::WeakPtrFactory<GeolocationArbitratorImpl> weak_ptr_factory_; - + CancelableRequestConsumer request_consumer_; bool use_mock_; }; GeolocationArbitratorImpl::GeolocationArbitratorImpl( - AccessTokenStoreFactory* access_token_store_factory, + AccessTokenStore* access_token_store, URLRequestContextGetter* context_getter) - : access_token_store_factory_(access_token_store_factory), + : access_token_store_(access_token_store), context_getter_(context_getter), default_url_(kDefaultNetworkProviderUrl), - ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)), use_mock_(false) { DCHECK(default_url_.is_valid()); } @@ -123,22 +119,23 @@ void GeolocationArbitratorImpl::MovementDetected( DCHECK(CalledOnValidThread()); } -void GeolocationArbitratorImpl::OnAccessTokenStoresCreated( - const AccessTokenStoreFactory::TokenStoreSet& access_token_store) { - DCHECK(provider_ == NULL); - // TODO(joth): Once we have arbitration implementation, iterate the whole set - // rather than cherry-pick our defaul url. - AccessTokenStoreFactory::TokenStoreSet::const_iterator item = - access_token_store.find(default_url_); - DCHECK(item != access_token_store.end()); - DCHECK(item->second != NULL); +void GeolocationArbitratorImpl::OnAccessTokenStoresLoaded( + AccessTokenStore::AccessTokenSet access_token_set) { + DCHECK(provider_ == NULL) + << "OnAccessTokenStoresLoaded : has existing location " + << "provider. Race condition caused repeat load of tokens?"; if (use_mock_) { provider_.reset(NewMockLocationProvider()); } else { + // TODO(joth): Once we have arbitration implementation, 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): Check with GLS folks what hostname they'd like sent. provider_.reset(NewNetworkLocationProvider( - item->second, context_getter_.get(), - default_url_, ASCIIToUTF16("chromium.org"))); + access_token_store_.get(), context_getter_.get(), default_url_, + token != access_token_set.end() ? token->second : string16(), + ASCIIToUTF16("chromium.org"))); } DCHECK(provider_ != NULL); provider_->RegisterListener(this); @@ -149,9 +146,11 @@ void GeolocationArbitratorImpl::OnAccessTokenStoresCreated( void GeolocationArbitratorImpl::CreateProviders() { DCHECK(CalledOnValidThread()); DCHECK(!observers_.empty()); - if (provider_ == NULL) { - access_token_store_factory_->CreateAccessTokenStores( - weak_ptr_factory_.GetWeakPtr(), default_url_); + if (provider_ == NULL && !request_consumer_.HasPendingRequests()) { + access_token_store_->LoadAccessTokens( + &request_consumer_, + NewCallback(this, + &GeolocationArbitratorImpl::OnAccessTokenStoresLoaded)); } // TODO(joth): Use high accuracy flag to conditionally create GPS provider. } @@ -167,9 +166,9 @@ bool GeolocationArbitratorImpl::HasHighAccuracyObserver() { } GeolocationArbitrator* GeolocationArbitrator::New( - AccessTokenStoreFactory* access_token_store_factory, + AccessTokenStore* access_token_store, URLRequestContextGetter* context_getter) { - return new GeolocationArbitratorImpl(access_token_store_factory, + return new GeolocationArbitratorImpl(access_token_store, context_getter); } diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h index b0c10bc9..df2e517 100644 --- a/chrome/browser/geolocation/location_arbitrator.h +++ b/chrome/browser/geolocation/location_arbitrator.h @@ -5,7 +5,7 @@ #ifndef CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_ #define CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_ -class AccessTokenStoreFactory; +class AccessTokenStore; class URLRequestContextGetter; struct Geoposition; @@ -21,7 +21,7 @@ class GeolocationArbitrator { public: // Creates and returns a new instance of the location arbitrator. static GeolocationArbitrator* New( - AccessTokenStoreFactory* access_token_store_factory, + AccessTokenStore* access_token_store, URLRequestContextGetter* context_getter); class Delegate { diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc index 49f3cb3..63b22bb 100644 --- a/chrome/browser/geolocation/location_arbitrator_unittest.cc +++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc @@ -41,8 +41,8 @@ void SetReferencePosition(Geoposition* position) { class GeolocationLocationArbitratorTest : public testing::Test { protected: virtual void SetUp() { - access_token_factory_ = new FakeAccessTokenStoreFactory; - arbitrator_.reset(GeolocationArbitrator::New(access_token_factory_.get(), + access_token_store_ = new FakeAccessTokenStore; + arbitrator_.reset(GeolocationArbitrator::New(access_token_store_.get(), NULL)); arbitrator_->SetUseMockProvider(true); } @@ -50,31 +50,31 @@ class GeolocationLocationArbitratorTest : public testing::Test { virtual void TearDown() { } - scoped_refptr<FakeAccessTokenStoreFactory> access_token_factory_; + scoped_refptr<FakeAccessTokenStore> access_token_store_; scoped_ptr<GeolocationArbitrator> arbitrator_; }; TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) { - EXPECT_TRUE(access_token_factory_); + EXPECT_TRUE(access_token_store_); EXPECT_TRUE(arbitrator_ != NULL); arbitrator_.reset(); SUCCEED(); } TEST_F(GeolocationLocationArbitratorTest, NormalUsage) { - ASSERT_TRUE(access_token_factory_); + ASSERT_TRUE(access_token_store_); ASSERT_TRUE(arbitrator_ != NULL); - EXPECT_FALSE(access_token_factory_->default_url_.is_valid()); - EXPECT_FALSE(access_token_factory_->delegate_); + EXPECT_TRUE(access_token_store_->access_token_set_.empty()); + EXPECT_FALSE(access_token_store_->request_); MockLocationObserver observer; arbitrator_->AddObserver(&observer, GeolocationArbitrator::UpdateOptions()); - EXPECT_TRUE(access_token_factory_->default_url_.is_valid()); - ASSERT_TRUE(access_token_factory_->delegate_); + EXPECT_TRUE(access_token_store_->access_token_set_.empty()); + ASSERT_TRUE(access_token_store_->request_); EXPECT_FALSE(MockLocationProvider::instance_); - access_token_factory_->NotifyDelegateStoreCreated(); + access_token_store_->NotifyDelegateTokensLoaded(); ASSERT_TRUE(MockLocationProvider::instance_); EXPECT_TRUE(MockLocationProvider::instance_->has_listeners()); EXPECT_EQ(1, MockLocationProvider::instance_->started_count_); @@ -95,7 +95,7 @@ TEST_F(GeolocationLocationArbitratorTest, MultipleListener) { MockLocationObserver observer2; arbitrator_->AddObserver(&observer2, GeolocationArbitrator::UpdateOptions()); - access_token_factory_->NotifyDelegateStoreCreated(); + access_token_store_->NotifyDelegateTokensLoaded(); ASSERT_TRUE(MockLocationProvider::instance_); EXPECT_FALSE(observer1.last_position_.IsInitialized()); EXPECT_FALSE(observer2.last_position_.IsInitialized()); diff --git a/chrome/browser/geolocation/location_provider.h b/chrome/browser/geolocation/location_provider.h index bacafc7..4d0c9b1 100644 --- a/chrome/browser/geolocation/location_provider.h +++ b/chrome/browser/geolocation/location_provider.h @@ -118,6 +118,7 @@ LocationProviderBase* NewNetworkLocationProvider( AccessTokenStore* access_token_store, URLRequestContextGetter* context, const GURL& url, + const string16& access_token, const string16& host_name); #endif // CHROME_BROWSER_GEOLOCATION_LOCATION_PROVIDER_H_ diff --git a/chrome/browser/geolocation/network_location_provider.cc b/chrome/browser/geolocation/network_location_provider.cc index 1b05018..21e536a 100644 --- a/chrome/browser/geolocation/network_location_provider.cc +++ b/chrome/browser/geolocation/network_location_provider.cc @@ -107,9 +107,10 @@ LocationProviderBase* NewNetworkLocationProvider( AccessTokenStore* access_token_store, URLRequestContextGetter* context, const GURL& url, + const string16& access_token, const string16& host_name) { - return new NetworkLocationProvider(access_token_store, context, - url, host_name); + return new NetworkLocationProvider( + access_token_store, context, url, access_token, host_name); } // NetworkLocationProvider @@ -117,6 +118,7 @@ NetworkLocationProvider::NetworkLocationProvider( AccessTokenStore* access_token_store, URLRequestContextGetter* url_context_getter, const GURL& url, + const string16& access_token, const string16& host_name) : access_token_store_(access_token_store), radio_data_provider_(NULL), @@ -124,6 +126,7 @@ NetworkLocationProvider::NetworkLocationProvider( is_radio_data_complete_(false), is_wifi_data_complete_(false), device_data_updated_timestamp_(kint64min), + access_token_(access_token), is_new_data_available_(false), ALLOW_THIS_IN_INITIALIZER_LIST(delayed_start_task_(this)) { // Create the position cache. @@ -193,7 +196,7 @@ void NetworkLocationProvider::LocationResponseAvailable( // Record access_token if it's set. if (!access_token.empty() && access_token_ != access_token) { access_token_ = access_token; - access_token_store_->SetAccessToken(access_token); + access_token_store_->SaveAccessToken(request_->url(), access_token); } // If new data arrived whilst request was pending reissue the request. @@ -270,9 +273,6 @@ void NetworkLocationProvider::RequestPosition() { is_new_data_available_ = false; - if (access_token_.empty()) - access_token_store_->GetAccessToken(&access_token_); - AutoLock data_lock(data_mutex_); request_->MakeRequest(access_token_, radio_data_, wifi_data_, device_data_updated_timestamp_); diff --git a/chrome/browser/geolocation/network_location_provider.h b/chrome/browser/geolocation/network_location_provider.h index 2988841..efcb20b 100644 --- a/chrome/browser/geolocation/network_location_provider.h +++ b/chrome/browser/geolocation/network_location_provider.h @@ -27,6 +27,7 @@ class NetworkLocationProvider NetworkLocationProvider(AccessTokenStore* access_token_store, URLRequestContextGetter* context, const GURL& url, + const string16& access_token, const string16& host_name); virtual ~NetworkLocationProvider(); diff --git a/chrome/browser/geolocation/network_location_provider_unittest.cc b/chrome/browser/geolocation/network_location_provider_unittest.cc index 34e6b2d..1b05f38 100644 --- a/chrome/browser/geolocation/network_location_provider_unittest.cc +++ b/chrome/browser/geolocation/network_location_provider_unittest.cc @@ -108,7 +108,7 @@ class GeolocationNetworkProviderTest : public testing::Test { public: virtual void SetUp() { URLFetcher::set_factory(&url_fetcher_factory_); - access_token_store_ = new FakeAccessTokenStore(test_server_url_); + access_token_store_ = new FakeAccessTokenStore; } virtual void TearDown() { @@ -123,6 +123,7 @@ class GeolocationNetworkProviderTest : public testing::Test { 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)); } @@ -292,11 +293,8 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { ResponseCookies(), kNoFixNetworkResponse); // This should have set the access token anyhow - EXPECT_EQ(access_token_store_->access_token_set_, - ASCIIToUTF16(REFERENCE_ACCESS_TOKEN)); - string16 token; - EXPECT_TRUE(access_token_store_->GetAccessToken(&token)); - EXPECT_EQ(REFERENCE_ACCESS_TOKEN, UTF16ToUTF8(token)); + EXPECT_EQ(UTF8ToUTF16(REFERENCE_ACCESS_TOKEN), + access_token_store_->access_token_set_[test_server_url_]); Geoposition position; provider->GetPosition(&position); @@ -339,8 +337,8 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) { EXPECT_TRUE(position.IsValidFix()); // Token should still be in the store. - EXPECT_TRUE(access_token_store_->GetAccessToken(&token)); - EXPECT_EQ(REFERENCE_ACCESS_TOKEN, UTF16ToUTF8(token)); + EXPECT_EQ(UTF8ToUTF16(REFERENCE_ACCESS_TOKEN), + access_token_store_->access_token_set_[test_server_url_]); // Wifi updated again, with one less AP. This is 'close enough' to the // previous scan, so no new request made. |