Remove use of GetDefaultRequestContext in geolocation.

Now uses the ChromeAccessTokenStore to get us a system request context from within the chrome/ directory. Also access tokens and the context getter are reloaded each time GeolocationArbitrator::StartProviders is called if providers have not already been started.

BUG=92363
TEST=

Review URL: http://codereview.chromium.org/7809020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@99719 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 104 insertions, 65 deletions

diff --git a/content/browser/geolocation/access_token_store.h b/content/browser/geolocation/access_token_store.h
index 87ad577..df3ec61 100644
--- a/content/browser/geolocation/access_token_store.h
+++ b/content/browser/geolocation/access_token_store.h
@@ -24,6 +24,10 @@
 
 class GURL;
 
+namespace net {
+class URLRequestContextGetter;
+}
+
 // Provides storage for the access token used in the network request.
 class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore>,
                          public CancelableRequestProvider {
@@ -31,9 +35,14 @@ class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore>,
 
   // 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.
+  typedef Callback2<AccessTokenSet, net::URLRequestContextGetter*>::Type
+      LoadAccessTokensCallbackType;
+  // |callback| will be invoked once per LoadAccessTokens call, after existing
+  // access tokens have been loaded from persistent store. As a convenience the
+  // URLRequestContextGetter is also supplied as an argument in |callback|, as
+  // in Chrome the call to obtain this must also be performed on the UI thread
+  // so it is efficient to piggyback it onto this request.
+  // Takes ownership of |callback|.
   // Returns a handle which can subsequently be used with CancelRequest().
   Handle LoadAccessTokens(CancelableRequestConsumerBase* consumer,
                           LoadAccessTokensCallbackType* callback);
diff --git a/content/browser/geolocation/arbitrator_dependency_factory.cc b/content/browser/geolocation/arbitrator_dependency_factory.cc
index 252813d..a2f2790 100644
--- a/content/browser/geolocation/arbitrator_dependency_factory.cc
+++ b/content/browser/geolocation/arbitrator_dependency_factory.cc
@@ -15,13 +15,6 @@ GeolocationArbitratorDependencyFactory::
 }
 
 // DefaultGeolocationArbitratorDependencyFactory
-net::URLRequestContextGetter*
-DefaultGeolocationArbitratorDependencyFactory::GetContextGetter() {
-  // Deprecated; see http://crbug.com/92363
-  return content::GetContentClient()->browser()->
-      GetDefaultRequestContextDeprecatedCrBug64339();
-}
-
 DefaultGeolocationArbitratorDependencyFactory::GetTimeNow
 DefaultGeolocationArbitratorDependencyFactory::GetTimeFunction() {
   return base::Time::Now;
diff --git a/content/browser/geolocation/arbitrator_dependency_factory.h b/content/browser/geolocation/arbitrator_dependency_factory.h
index 1adaeaf..7114b70 100644
--- a/content/browser/geolocation/arbitrator_dependency_factory.h
+++ b/content/browser/geolocation/arbitrator_dependency_factory.h
@@ -30,7 +30,6 @@ class GeolocationArbitratorDependencyFactory
   typedef base::Time (*GetTimeNow)();
 
   virtual GetTimeNow GetTimeFunction() = 0;
-  virtual net::URLRequestContextGetter* GetContextGetter() = 0;
   virtual AccessTokenStore* NewAccessTokenStore() = 0;
   virtual LocationProviderBase* NewNetworkLocationProvider(
       AccessTokenStore* access_token_store,
@@ -50,7 +49,6 @@ class DefaultGeolocationArbitratorDependencyFactory
     : public GeolocationArbitratorDependencyFactory {
  public:
   // GeolocationArbitratorDependencyFactory
-  virtual net::URLRequestContextGetter* GetContextGetter();
   virtual GetTimeNow GetTimeFunction();
   virtual AccessTokenStore* NewAccessTokenStore();
   virtual LocationProviderBase* NewNetworkLocationProvider(
diff --git a/content/browser/geolocation/fake_access_token_store.cc b/content/browser/geolocation/fake_access_token_store.cc
index d61af54..548c127 100644
--- a/content/browser/geolocation/fake_access_token_store.cc
+++ b/content/browser/geolocation/fake_access_token_store.cc
@@ -4,22 +4,33 @@
 
 #include "content/browser/geolocation/fake_access_token_store.h"
 
-FakeAccessTokenStore::FakeAccessTokenStore() {}
+using testing::_;
+using testing::Invoke;
+
+FakeAccessTokenStore::FakeAccessTokenStore() {
+  ON_CALL(*this, DoLoadAccessTokens(_))
+      .WillByDefault(Invoke(this,
+                            &FakeAccessTokenStore::DefaultDoLoadAccessTokens));
+  ON_CALL(*this, SaveAccessToken(_, _))
+      .WillByDefault(Invoke(this,
+                            &FakeAccessTokenStore::DefaultSaveAccessToken));
+}
 
 void FakeAccessTokenStore::NotifyDelegateTokensLoaded() {
   CHECK(request_ != NULL);
-  request_->ForwardResult(MakeTuple(access_token_set_));
+  net::URLRequestContextGetter* context_getter = NULL;
+  request_->ForwardResult(MakeTuple(access_token_set_, context_getter));
   request_ = NULL;
 }
 
-void FakeAccessTokenStore::DoLoadAccessTokens(
+void FakeAccessTokenStore::DefaultDoLoadAccessTokens(
     scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) {
   DCHECK(request_ == NULL)
       << "Fake token store currently only allows one request at a time";
   request_ = request;
 }
 
-void FakeAccessTokenStore::SaveAccessToken(
+void FakeAccessTokenStore::DefaultSaveAccessToken(
     const GURL& server_url, const string16& access_token) {
   DCHECK(server_url.is_valid());
   access_token_set_[server_url] = access_token;
diff --git a/content/browser/geolocation/fake_access_token_store.h b/content/browser/geolocation/fake_access_token_store.h
index 2c31aa2..a3ee08a 100644
--- a/content/browser/geolocation/fake_access_token_store.h
+++ b/content/browser/geolocation/fake_access_token_store.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -7,6 +7,7 @@
 #pragma once
 
 #include "content/browser/geolocation/access_token_store.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 // A fake (non-persisted) access token store instance useful for testing.
@@ -17,10 +18,17 @@ class FakeAccessTokenStore : public AccessTokenStore {
   void NotifyDelegateTokensLoaded();
 
   // AccessTokenStore
-  virtual void DoLoadAccessTokens(
+  MOCK_METHOD1(DoLoadAccessTokens,
+               void(scoped_refptr<CancelableRequest
+                    <LoadAccessTokensCallbackType> > request));
+  MOCK_METHOD2(SaveAccessToken,
+               void(const GURL& server_url, const string16& access_token));
+
+  void DefaultDoLoadAccessTokens(
       scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request);
-  virtual void SaveAccessToken(
-      const GURL& server_url, const string16& access_token);
+
+  void DefaultSaveAccessToken(const GURL& server_url,
+                              const string16& access_token);
 
   AccessTokenSet access_token_set_;
   scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request_;
diff --git a/content/browser/geolocation/geolocation_provider.h b/content/browser/geolocation/geolocation_provider.h
index c955a3c..0f3db353 100644
--- a/content/browser/geolocation/geolocation_provider.h
+++ b/content/browser/geolocation/geolocation_provider.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -18,7 +18,7 @@ class GeolocationArbitrator;
 template<typename Type>
 struct DefaultSingletonTraits;
 
-// This is the main API to the geolocaiton subsystem. The application
+// This is the main API to the geolocation subsystem. The application
 // will hold a single instance of this class, and can register multiple
 // observers which will be notified of location updates. Underlying location
 // arbitrator will only be enabled whilst there is at least one observer
diff --git a/content/browser/geolocation/geolocation_provider_unittest.cc b/content/browser/geolocation/geolocation_provider_unittest.cc
index da6317d..f606a96 100644
--- a/content/browser/geolocation/geolocation_provider_unittest.cc
+++ b/content/browser/geolocation/geolocation_provider_unittest.cc
@@ -3,13 +3,21 @@
 // found in the LICENSE file.
 
 #include "base/memory/singleton.h"
+#include "base/synchronization/waitable_event.h"
 #include "content/browser/geolocation/arbitrator_dependency_factories_for_test.h"
 #include "content/browser/geolocation/fake_access_token_store.h"
 #include "content/browser/geolocation/geolocation_provider.h"
 #include "content/browser/geolocation/location_arbitrator.h"
 #include "content/browser/geolocation/mock_location_provider.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::DoAll;
+using testing::DoDefault;
+using testing::Invoke;
+using testing::InvokeWithoutArgs;
+
 namespace {
 
 class GeolocationProviderTest : public testing::Test {
@@ -68,6 +76,12 @@ class StartStopMockLocationProvider : public MockLocationProvider {
         test_loop_(test_loop) {
   }
 
+  virtual ~StartStopMockLocationProvider() {
+    Die();
+  }
+
+  MOCK_METHOD0(Die, void());
+
   virtual bool StartProvider(bool high_accuracy) {
     bool result = MockLocationProvider::StartProvider(high_accuracy);
     test_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask);
@@ -125,6 +139,13 @@ TEST_F(GeolocationProviderTest, StartStop) {
       new FakeAccessTokenStore;
   scoped_refptr<GeolocationArbitratorDependencyFactory> dependency_factory =
       new MockDependencyFactory(&message_loop_, fake_access_token_store.get());
+  base::WaitableEvent event(false, false);
+
+  EXPECT_CALL(*(fake_access_token_store.get()), DoLoadAccessTokens(_))
+      .Times(1)
+      .WillOnce(DoAll(Invoke(fake_access_token_store.get(),
+                             &FakeAccessTokenStore::DefaultDoLoadAccessTokens),
+                      InvokeWithoutArgs(&event, &base::WaitableEvent::Signal)));
 
   GeolocationArbitrator::SetDependencyFactoryForTest(dependency_factory.get());
 
@@ -132,17 +153,25 @@ TEST_F(GeolocationProviderTest, StartStop) {
   NullGeolocationObserver null_observer;
   GeolocationObserverOptions options;
   provider_->AddObserver(&null_observer, options);
+  EXPECT_TRUE(provider_->IsRunning());
+  // Wait for token load request from the arbitrator to come through.
+  event.Wait();
+  event.Reset();
   // The GeolocationArbitrator won't start the providers until it has
   // finished loading access tokens.
   fake_access_token_store->NotifyDelegateTokensLoaded();
-  EXPECT_TRUE(provider_->IsRunning());
   message_loop_.Run();
   EXPECT_EQ(MockLocationProvider::instance_->state_,
             MockLocationProvider::LOW_ACCURACY);
+  EXPECT_CALL(*(static_cast<StartStopMockLocationProvider*>(
+                  MockLocationProvider::instance_)),
+              Die())
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(&event, &base::WaitableEvent::Signal));
   provider_->RemoveObserver(&null_observer);
-  message_loop_.Run();
-  EXPECT_EQ(MockLocationProvider::instance_->state_,
-            MockLocationProvider::STOPPED);
+  // Wait for the providers to be stopped.
+  event.Wait();
+  event.Reset();
   EXPECT_TRUE(provider_->IsRunning());
 
   GeolocationArbitrator::SetDependencyFactoryForTest(NULL);
diff --git a/content/browser/geolocation/location_arbitrator.cc b/content/browser/geolocation/location_arbitrator.cc
index 6905ede..0160e4e 100644
--- a/content/browser/geolocation/location_arbitrator.cc
+++ b/content/browser/geolocation/location_arbitrator.cc
@@ -25,15 +25,10 @@ GeolocationArbitrator::GeolocationArbitrator(
     GeolocationObserver* observer)
     : dependency_factory_(dependency_factory),
       access_token_store_(dependency_factory->NewAccessTokenStore()),
-      context_getter_(dependency_factory->GetContextGetter()),
       get_time_now_(dependency_factory->GetTimeFunction()),
       observer_(observer),
       position_provider_(NULL) {
-  DCHECK(GURL(kDefaultNetworkProviderUrl).is_valid());
-  access_token_store_->LoadAccessTokens(
-      &request_consumer_,
-      NewCallback(this,
-                  &GeolocationArbitrator::OnAccessTokenStoresLoaded));
+
 }
 
 GeolocationArbitrator::~GeolocationArbitrator() {
@@ -62,13 +57,20 @@ void GeolocationArbitrator::OnPermissionGranted(
 
 void GeolocationArbitrator::StartProviders(
     const GeolocationObserverOptions& options) {
-  // Stash options incase OnAccessTokenStoresLoaded has not yet been called
-  // (in which case |providers_| will be empty).
+  // Stash options as OnAccessTokenStoresLoaded has not yet been called.
   current_provider_options_ = options;
-  StartProviders();
+  if (providers_.empty()) {
+    DCHECK(GURL(kDefaultNetworkProviderUrl).is_valid());
+    access_token_store_->LoadAccessTokens(
+        &request_consumer_,
+        NewCallback(this,
+                    &GeolocationArbitrator::OnAccessTokenStoresLoaded));
+  } else {
+    DoStartProviders();
+  }
 }
 
-void GeolocationArbitrator::StartProviders() {
+void GeolocationArbitrator::DoStartProviders() {
   for (ScopedVector<LocationProviderBase>::iterator i = providers_.begin();
        i != providers_.end(); ++i) {
     (*i)->StartProvider(current_provider_options_.use_high_accuracy);
@@ -76,17 +78,17 @@ void GeolocationArbitrator::StartProviders() {
 }
 
 void GeolocationArbitrator::StopProviders() {
-  for (ScopedVector<LocationProviderBase>::iterator i = providers_.begin();
-       i != providers_.end(); ++i) {
-    (*i)->StopProvider();
-  }
+  providers_.reset();
 }
 
 void GeolocationArbitrator::OnAccessTokenStoresLoaded(
-    AccessTokenStore::AccessTokenSet access_token_set) {
-  DCHECK(providers_.empty())
-      << "OnAccessTokenStoresLoaded : has existing location "
-      << "provider. Race condition caused repeat load of tokens?";
+    AccessTokenStore::AccessTokenSet access_token_set,
+    net::URLRequestContextGetter* context_getter) {
+  if (!providers_.empty()) {
+    // A second StartProviders() call may have arrived before the first
+    // completed.
+    return;
+  }
   // If there are no access tokens, boot strap it with the default server URL.
   if (access_token_set.empty())
     access_token_set[GURL(kDefaultNetworkProviderUrl)];
@@ -95,11 +97,11 @@ void GeolocationArbitrator::OnAccessTokenStoresLoaded(
       i != access_token_set.end(); ++i) {
     RegisterProvider(
         dependency_factory_->NewNetworkLocationProvider(
-            access_token_store_.get(), context_getter_.get(),
+            access_token_store_.get(), context_getter,
             i->first, i->second));
   }
   RegisterProvider(dependency_factory_->NewSystemLocationProvider());
-  StartProviders();
+  DoStartProviders();
 }
 
 void GeolocationArbitrator::RegisterProvider(
diff --git a/content/browser/geolocation/location_arbitrator.h b/content/browser/geolocation/location_arbitrator.h
index 84522eb..5370cd1 100644
--- a/content/browser/geolocation/location_arbitrator.h
+++ b/content/browser/geolocation/location_arbitrator.h
@@ -75,8 +75,9 @@ class GeolocationArbitrator : public LocationProviderBase::ListenerInterface {
   // |providers_| or deleted on error (e.g. it fails to start).
   void RegisterProvider(LocationProviderBase* provider);
   void OnAccessTokenStoresLoaded(
-      AccessTokenStore::AccessTokenSet access_token_store);
-  void StartProviders();
+      AccessTokenStore::AccessTokenSet access_token_store,
+      net::URLRequestContextGetter* context_getter);
+  void DoStartProviders();
   // 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
   // provider.
@@ -86,7 +87,6 @@ class GeolocationArbitrator : public LocationProviderBase::ListenerInterface {
 
   scoped_refptr<GeolocationArbitratorDependencyFactory> dependency_factory_;
   scoped_refptr<AccessTokenStore> access_token_store_;
-  scoped_refptr<net::URLRequestContextGetter> context_getter_;
   GetTimeNow get_time_now_;
   GeolocationObserver* observer_;
   ScopedVector<LocationProviderBase> providers_;
diff --git a/content/browser/geolocation/location_arbitrator_unittest.cc b/content/browser/geolocation/location_arbitrator_unittest.cc
index 327af85..b87b600 100644
--- a/content/browser/geolocation/location_arbitrator_unittest.cc
+++ b/content/browser/geolocation/location_arbitrator_unittest.cc
@@ -68,9 +68,6 @@ class MockDependencyFactory : public GeolocationArbitratorDependencyFactory {
   virtual GeolocationArbitrator::GetTimeNow GetTimeFunction() {
     return GetTimeNowForTest;
   }
-  virtual net::URLRequestContextGetter* GetContextGetter() {
-    return NULL;
-  }
   virtual AccessTokenStore* NewAccessTokenStore() {
     return access_token_store_.get();
   }
@@ -165,13 +162,15 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   ASSERT_TRUE(access_token_store_);
   ASSERT_TRUE(arbitrator_ != NULL);
 
+  EXPECT_FALSE(cell());
+  EXPECT_FALSE(gps());
+  arbitrator_->StartProviders(GeolocationObserverOptions(false));
+
   EXPECT_TRUE(access_token_store_->access_token_set_.empty());
   EXPECT_TRUE(access_token_store_->request_);
   EXPECT_TRUE(access_token_store_->access_token_set_.empty());
   ASSERT_TRUE(access_token_store_->request_);
 
-  EXPECT_FALSE(cell());
-  EXPECT_FALSE(gps());
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   EXPECT_TRUE(gps());
@@ -199,18 +198,7 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
-  access_token_store_->NotifyDelegateTokensLoaded();
-  ASSERT_TRUE(cell());
-  ASSERT_TRUE(gps());
-  arbitrator_->StartProviders(GeolocationObserverOptions(true));
-  EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, cell()->state_);
-  EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, gps()->state_);
   arbitrator_->StartProviders(GeolocationObserverOptions(false));
-  EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
-  EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
-}
-
-TEST_F(GeolocationLocationArbitratorTest, StartProvidersAfterFixArrives) {
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
@@ -225,6 +213,7 @@ TEST_F(GeolocationLocationArbitratorTest, StartProvidersAfterFixArrives) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
+  arbitrator_->StartProviders(GeolocationObserverOptions(false));
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());