Add unit tests for locaiton arbitrator and browser tests for the access token store (as it integrates with browser singletons)

Fixes a few bugs discovered along the way, and make large-ish redesign of the access token API in order to allow sane use with threading restrictions.

BUG=None 
TEST=unit_tests.exe --gtest_filter=Geol* --gtest_repeat=10000 --gtest_break_on_failure 

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

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

14 files changed, 634 insertions, 104 deletions

diff --git a/chrome/browser/geolocation/access_token_store.cc b/chrome/browser/geolocation/access_token_store.cc
index 0055103..9c5388f 100644
--- a/chrome/browser/geolocation/access_token_store.cc
+++ b/chrome/browser/geolocation/access_token_store.cc
@@ -7,45 +7,152 @@
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
 #include "googleurl/src/gurl.h"
 
 namespace {
-// Implementation of the geolocation access token store using chrome's prefs
-// store (local_state) to persist these tokens.
-// TODO(joth): Calls APIs which probably can't be used from the thread that it
-// will be run on; needs prefs access bounced via UI thread.
 class ChromePrefsAccessTokenStore : public AccessTokenStore {
  public:
+  ChromePrefsAccessTokenStore(
+      const GURL& url, bool token_valid, const string16& initial_access_token);
+
+ private:
   // AccessTokenStore
-  virtual bool SetAccessToken(const GURL& url,
-                              const string16& access_token) {
-    DictionaryValue* access_token_dictionary =
-        g_browser_process->local_state()->GetMutableDictionary(
-            prefs::kGeolocationAccessToken);
-    access_token_dictionary->SetWithoutPathExpansion(
-        UTF8ToWide(url.spec()),
-        Value::CreateStringValueFromUTF16(access_token));
-    return true;
+  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);
+
+  // AccessTokenStoreFactory
+  virtual void CreateAccessTokenStores(
+      const base::WeakPtr<AccessTokenStoreFactory::Delegate>& delegate,
+      const GURL& default_url);
+
+  base::WeakPtr<AccessTokenStoreFactory::Delegate> delegate_;
+};
+
+ChromePrefsAccessTokenStore::ChromePrefsAccessTokenStore(
+      const GURL& url, bool token_valid, const string16& initial_access_token)
+    : AccessTokenStore(url, token_valid, initial_access_token) {
+}
+
+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));
+}
 
-  virtual bool GetAccessToken(const GURL& url, string16* access_token) {
-    const DictionaryValue* access_token_dictionary =
-        g_browser_process->local_state()->GetDictionary(
-            prefs::kGeolocationAccessToken);
-    // Careful: The returned value could be NULL if the pref has never been set.
-    if (access_token_dictionary == NULL)
-      return false;
-    return access_token_dictionary->GetStringAsUTF16WithoutPathExpansion(
-        UTF8ToWide(url.spec()), access_token);
+void ChromePrefsAccessTokenStoreFactory::LoadDictionaryStoreInUIThread(
+    ChromeThread::ID client_thread_id, const GURL& default_url) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  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.
+  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())
+        continue;
+      string16 token;
+      const bool token_valid =
+          token_dictionary->GetStringAsUTF16WithoutPathExpansion(*it, &token);
+      token_stores[this_url] =
+          new ChromePrefsAccessTokenStore(this_url, token_valid, token);
+    }
   }
-};
+  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));
+}
 
+void ChromePrefsAccessTokenStoreFactory::NotifyDelegateInClientThread(
+    const TokenStoreSet& token_stores) {
+  if (delegate_ != NULL) {
+    delegate_->OnAccessTokenStoresCreated(token_stores);
+  }
+}
+
+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));
+}
 }  // namespace
 
 void AccessTokenStore::RegisterPrefs(PrefService* prefs) {
   prefs->RegisterDictionaryPref(prefs::kGeolocationAccessToken);
 }
 
-AccessTokenStore* NewChromePrefsAccessTokenStore();
+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() {
+}
+
+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);
+}
+
+bool AccessTokenStore::GetAccessToken(string16* access_token) const {
+  DCHECK(access_token);
+  *access_token = access_token_;
+  return access_token_valid_;
+}
+
+AccessTokenStoreFactory::~AccessTokenStoreFactory() {
+}
+
+// Creates a new access token store backed by the global chome prefs.
+AccessTokenStoreFactory* NewChromePrefsAccessTokenStoreFactory() {
+  return new ChromePrefsAccessTokenStoreFactory;
+}
diff --git a/chrome/browser/geolocation/access_token_store.h b/chrome/browser/geolocation/access_token_store.h
index 11733a4..4a081d8 100644
--- a/chrome/browser/geolocation/access_token_store.h
+++ b/chrome/browser/geolocation/access_token_store.h
@@ -2,32 +2,79 @@
 // 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.
+// 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
+// of clients, without dependency on browser process singleton objects etc.
+
 #ifndef CHROME_BROWSER_GEOLOCATION_ACCESS_TOKEN_STORE_H_
 #define CHROME_BROWSER_GEOLOCATION_ACCESS_TOKEN_STORE_H_
 
+#include <map>
+
 #include "base/ref_counted.h"
 #include "base/string16.h"
+#include "base/weak_ptr.h"
+#include "googleurl/src/gurl.h"
 
 class GURL;
 class PrefService;
 
 // Provides storage for the access token used in the network request.
-// Provided to allow for mocking for testing, by decoupling deep browser
-// dependencies (singleton prefs & threads) from the geolocaiton object.
-class AccessTokenStore : public base::RefCounted<AccessTokenStore> {
+class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore> {
  public:
   static void RegisterPrefs(PrefService* prefs);
 
-  virtual bool SetAccessToken(const GURL& url,
-                              const string16& access_token) = 0;
-  virtual bool GetAccessToken(const GURL& url, string16* access_token) = 0;
+  GURL server_url() const;
+  void SetAccessToken(const string16& access_token);
+  bool GetAccessToken(string16* access_token) const;
+
+ protected:
+  friend class base::RefCountedThreadSafe<AccessTokenStore>;
+  AccessTokenStore(
+      const GURL& url, bool token_valid, const string16& initial_access_token);
+  virtual ~AccessTokenStore();
+
+  virtual void DoSetAccessToken(const string16& access_token) = 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::RefCounted<AccessTokenStore>;
-  virtual ~AccessTokenStore() {}
+  friend class base::RefCountedThreadSafe<AccessTokenStoreFactory>;
+  ~AccessTokenStoreFactory();
 };
 
 // Creates a new access token store backed by the global chome prefs.
-AccessTokenStore* NewChromePrefsAccessTokenStore();
+AccessTokenStoreFactory* NewChromePrefsAccessTokenStoreFactory();
 
 #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
new file mode 100644
index 0000000..31b62fc
--- /dev/null
+++ b/chrome/browser/geolocation/access_token_store_browsertest.cc
@@ -0,0 +1,114 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/geolocation/access_token_store.h"
+
+#include "chrome/browser/chrome_thread.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+
+namespace {
+// The token store factory implementation expects to be used from any well-known
+// chrome thread other than UI. We could use any arbitrary thread; IO is
+// a good choice as this is the expected usage.
+const ChromeThread::ID kExpectedClientThreadId = ChromeThread::IO;
+const char* kRefServerUrl1 = "https://test.domain.example/foo?id=bar.bar";
+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:
+  GeolocationAccessTokenStoreTest()
+      : token_to_expect_(NULL), token_to_set_(NULL) {}
+
+  void StartThreadAndWaitForResults(
+      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);
+  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) {
+  ASSERT_TRUE(ChromeThread::CurrentlyOn(kExpectedClientThreadId));
+
+  scoped_refptr<AccessTokenStoreFactory> store =
+      NewChromePrefsAccessTokenStoreFactory();
+  store->CreateAccessTokenStores(delegate->AsWeakPtr(), ref_url);
+}
+}  // namespace
+
+void GeolocationAccessTokenStoreTest::StartThreadAndWaitForResults(
+    const char* ref_url, const string16* token_to_expect,
+    const string16* token_to_set) {
+  ref_url_ = GURL(ref_url);
+  token_to_expect_ = token_to_expect;
+  token_to_set_ = token_to_set;
+
+  ChromeThread::PostTask(
+      kExpectedClientThreadId, FROM_HERE, NewRunnableFunction(
+          &StartTestFromClientThread, this, ref_url_));
+  ui_test_utils::RunMessageLoop();
+}
+
+void GeolocationAccessTokenStoreTest::OnAccessTokenStoresCreated(
+      const AccessTokenStoreFactory::TokenStoreSet& access_token_store) {
+  ASSERT_TRUE(ChromeThread::CurrentlyOn(kExpectedClientThreadId))
+      << "Callback from token factory should be from the same thread as the "
+         "CreateAccessTokenStores 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);
+  if (!token_to_expect_) {
+    EXPECT_FALSE(read_ok);
+    EXPECT_TRUE(token.empty());
+  } else {
+    ASSERT_TRUE(read_ok);
+    EXPECT_EQ(*token_to_expect_, token);
+  }
+
+  if (token_to_set_) {
+    store->SetAccessToken(*token_to_set_);
+  }
+  ChromeThread::PostTask(
+      ChromeThread::UI, FROM_HERE, new MessageLoop::QuitTask);
+}
+
+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);
+  // Check it was set, and change to new value.
+  StartThreadAndWaitForResults(kRefServerUrl1, &ref_token1, &ref_token2);
+  // And change back.
+  StartThreadAndWaitForResults(kRefServerUrl1, &ref_token2, &ref_token1);
+  StartThreadAndWaitForResults(kRefServerUrl1, &ref_token1, NULL);
+
+  // Set a second server URL
+  StartThreadAndWaitForResults(kRefServerUrl2, NULL, &ref_token2);
+  StartThreadAndWaitForResults(kRefServerUrl2, &ref_token2, NULL);
+  StartThreadAndWaitForResults(kRefServerUrl1, &ref_token1, NULL);
+}
diff --git a/chrome/browser/geolocation/fake_access_token_store.h b/chrome/browser/geolocation/fake_access_token_store.h
new file mode 100644
index 0000000..767142e
--- /dev/null
+++ b/chrome/browser/geolocation/fake_access_token_store.h
@@ -0,0 +1,48 @@
+// Copyright (c) 2010 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.
+
+#ifndef CHROME_BROWSER_GEOLOCATION_FAKE_ACCESS_TOKEN_STORE_H_
+#define CHROME_BROWSER_GEOLOCATION_FAKE_ACCESS_TOKEN_STORE_H_
+
+#include "chrome/browser/geolocation/access_token_store.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// 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()) {}
+
+  virtual void DoSetAccessToken(const string16& access_token) {
+    access_token_set_ = access_token;
+  }
+
+  string16 access_token_set_;
+
+ private:
+  ~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_;
+};
+
+#endif  // CHROME_BROWSER_GEOLOCATION_FAKE_ACCESS_TOKEN_STORE_H_
diff --git a/chrome/browser/geolocation/geolocation_prefs.cc b/chrome/browser/geolocation/geolocation_prefs.cc
new file mode 100644
index 0000000..c99a18f
--- /dev/null
+++ b/chrome/browser/geolocation/geolocation_prefs.cc
@@ -0,0 +1,14 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/geolocation/geolocation_prefs.h"
+
+#include "chrome/browser/geolocation/access_token_store.h"
+
+namespace geolocation {
+void RegisterPrefs(PrefService* prefs) {
+  // Fan out to all geolocation sub-components that use prefs.
+  AccessTokenStore::RegisterPrefs(prefs);
+}
+}  // namespace geolocation
diff --git a/chrome/browser/geolocation/geolocation_prefs.h b/chrome/browser/geolocation/geolocation_prefs.h
new file mode 100644
index 0000000..3873274
--- /dev/null
+++ b/chrome/browser/geolocation/geolocation_prefs.h
@@ -0,0 +1,14 @@
+// Copyright (c) 2010 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.
+
+#ifndef CHROME_BROWSER_GEOLOCATION_GEOLOCATION_PREFS_H_
+#define CHROME_BROWSER_GEOLOCATION_GEOLOCATION_PREFS_H_
+
+class PrefService;
+
+namespace geolocation {
+void RegisterPrefs(PrefService* prefs);
+}
+
+#endif  // CHROME_BROWSER_GEOLOCATION_GEOLOCATION_PREFS_H_
diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc
index 6aa7939..04fba9d 100644
--- a/chrome/browser/geolocation/location_arbitrator.cc
+++ b/chrome/browser/geolocation/location_arbitrator.cc
@@ -18,80 +18,85 @@
 #include "googleurl/src/gurl.h"
 
 namespace {
-
 const char* kDefaultNetworkProviderUrl = "https://www.google.com/loc/json";
+}  // namespace
 
 class GeolocationArbitratorImpl
     : public GeolocationArbitrator,
       public LocationProviderBase::ListenerInterface,
+      public AccessTokenStoreFactory::Delegate,
       public NonThreadSafe {
  public:
-  GeolocationArbitratorImpl(AccessTokenStore* access_token_store,
+  GeolocationArbitratorImpl(AccessTokenStoreFactory* access_token_store_factory,
                             URLRequestContextGetter* context_getter);
   ~GeolocationArbitratorImpl();
 
   // GeolocationArbitrator
-  virtual void AddObserver(Delegate* delegate,
+  virtual void AddObserver(GeolocationArbitrator::Delegate* delegate,
                            const UpdateOptions& update_options);
-  virtual void RemoveObserver(Delegate* delegate);
+  virtual bool RemoveObserver(GeolocationArbitrator::Delegate* delegate);
   virtual void SetUseMockProvider(bool use_mock);
 
   // ListenerInterface
   virtual void LocationUpdateAvailable(LocationProviderBase* provider);
   virtual void MovementDetected(LocationProviderBase* provider);
 
+  // AccessTokenStoreFactory::Delegate
+  virtual void OnAccessTokenStoresCreated(
+      const AccessTokenStoreFactory::TokenStoreSet& access_token_store);
+
  private:
   void CreateProviders();
   bool HasHighAccuracyObserver();
 
-  scoped_refptr<AccessTokenStore> access_token_store_;
+  scoped_refptr<AccessTokenStoreFactory> access_token_store_factory_;
   scoped_refptr<URLRequestContextGetter> context_getter_;
 
   const GURL default_url_;
   scoped_ptr<LocationProviderBase> provider_;
 
-  typedef std::map<Delegate*, UpdateOptions> DelegateMap;
+  typedef std::map<GeolocationArbitrator::Delegate*, UpdateOptions> DelegateMap;
   DelegateMap observers_;
-  bool use_mock_;
-};
-
-}  // namespace
 
-GeolocationArbitrator* GeolocationArbitrator::New(
-    AccessTokenStore* access_token_store,
-    URLRequestContextGetter* context_getter) {
-  return new GeolocationArbitratorImpl(access_token_store, context_getter);
-}
+  base::WeakPtrFactory<GeolocationArbitratorImpl> weak_ptr_factory_;
 
-GeolocationArbitrator::~GeolocationArbitrator() {
-}
+  bool use_mock_;
+};
 
 GeolocationArbitratorImpl::GeolocationArbitratorImpl(
-    AccessTokenStore* access_token_store,
+    AccessTokenStoreFactory* access_token_store_factory,
     URLRequestContextGetter* context_getter)
-    : access_token_store_(access_token_store), context_getter_(context_getter),
-      default_url_(kDefaultNetworkProviderUrl), use_mock_(false) {
+    : access_token_store_factory_(access_token_store_factory),
+      context_getter_(context_getter),
+      default_url_(kDefaultNetworkProviderUrl),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      use_mock_(false) {
   DCHECK(default_url_.is_valid());
 }
 
 GeolocationArbitratorImpl::~GeolocationArbitratorImpl() {
   DCHECK(CalledOnValidThread());
+  DCHECK(observers_.empty()) << "Not all observers have unregistered";
 }
 
 void GeolocationArbitratorImpl::AddObserver(
-    Delegate* delegate, const UpdateOptions& update_options) {
+    GeolocationArbitrator::Delegate* delegate,
+    const UpdateOptions& update_options) {
   DCHECK(CalledOnValidThread());
   observers_[delegate] = update_options;
   CreateProviders();
 }
 
-void GeolocationArbitratorImpl::RemoveObserver(Delegate* delegate) {
+bool GeolocationArbitratorImpl::RemoveObserver(
+    GeolocationArbitrator::Delegate* delegate) {
   DCHECK(CalledOnValidThread());
-  observers_.erase(delegate);
-  if (observers_.empty()) {
+  size_t remove = observers_.erase(delegate);
+  if (observers_.empty() && provider_ != NULL) {
     // TODO(joth): Delayed callback to linger before destroying the provider.
+    provider_->UnregisterListener(this);
     provider_.reset();
   }
+  return remove > 0;
 }
 
 void GeolocationArbitratorImpl::SetUseMockProvider(bool use_mock) {
@@ -118,15 +123,37 @@ 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);
+  if (use_mock_) {
+    provider_.reset(NewMockLocationProvider());
+  } else {
+    // 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")));
+  }
+  DCHECK(provider_ != NULL);
+  provider_->RegisterListener(this);
+  const bool ok = provider_->StartProvider();
+  DCHECK(ok);
+}
+
 void GeolocationArbitratorImpl::CreateProviders() {
   DCHECK(CalledOnValidThread());
   DCHECK(!observers_.empty());
   if (provider_ == NULL) {
-    // TODO(joth): Check with GLS folks what hostname they'd like sent.
-    provider_.reset(NewNetworkLocationProvider(access_token_store_.get(),
-        context_getter_.get(), default_url_, ASCIIToUTF16("chromium.org")));
+    access_token_store_factory_->CreateAccessTokenStores(
+        weak_ptr_factory_.GetWeakPtr(), default_url_);
   }
-  // TODO(joth): Use high-accuracy to create conditionally create GPS provider.
+  // TODO(joth): Use high accuracy flag to conditionally create GPS provider.
 }
 
 bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
@@ -138,3 +165,13 @@ bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
   }
   return false;
 }
+
+GeolocationArbitrator* GeolocationArbitrator::New(
+    AccessTokenStoreFactory* access_token_store_factory,
+    URLRequestContextGetter* context_getter) {
+  return new GeolocationArbitratorImpl(access_token_store_factory,
+                                       context_getter);
+}
+
+GeolocationArbitrator::~GeolocationArbitrator() {
+}
diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h
index 46ab448..aaf61a0 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 AccessTokenStore;
+class AccessTokenStoreFactory;
 class URLRequestContextGetter;
 struct Position;
 
@@ -20,8 +20,9 @@ struct Position;
 class GeolocationArbitrator {
  public:
   // Creates and returns a new instance of the location arbitrator.
-  static GeolocationArbitrator* New(AccessTokenStore* access_token_store,
-                                    URLRequestContextGetter* context_getter);
+  static GeolocationArbitrator* New(
+      AccessTokenStoreFactory* access_token_store_factory,
+      URLRequestContextGetter* context_getter);
 
   class Delegate {
    public:
@@ -50,8 +51,8 @@ class GeolocationArbitrator {
   virtual void AddObserver(Delegate* delegate,
                            const UpdateOptions& update_options) = 0;
   // Remove a previously registered observer. No-op if not previously registered
-  // via AddObserver()
-  virtual void RemoveObserver(Delegate* delegate) = 0;
+  // via AddObserver(). Returns true if the observer was removed.
+  virtual bool RemoveObserver(Delegate* delegate) = 0;
 
   // TODO(joth): This is a stop-gap for testing; once we have decoupled
   // provider factory we should extract mock creation from the arbitrator.
diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc
new file mode 100644
index 0000000..6e0c1d8
--- /dev/null
+++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc
@@ -0,0 +1,171 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/geolocation/location_arbitrator.h"
+
+#include "base/scoped_ptr.h"
+#include "chrome/browser/geolocation/fake_access_token_store.h"
+#include "chrome/browser/geolocation/geoposition.h"
+#include "chrome/browser/geolocation/location_provider.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+// Mock implementation of a location provider for testing.
+class MockLocationProvider : public LocationProviderBase {
+ public:
+  MockLocationProvider() : started_count_(0) {
+    CHECK(instance_ == NULL);
+    instance_ = this;
+  }
+  virtual ~MockLocationProvider() {
+    CHECK(instance_ == this);
+    instance_ = NULL;
+  }
+
+  using LocationProviderBase::UpdateListeners;
+  using LocationProviderBase::InformListenersOfMovement;
+
+  // LocationProviderBase implementation.
+  virtual bool StartProvider() {
+    ++started_count_;
+    return true;
+  }
+  virtual void GetPosition(Position *position) {
+    *position = position_;
+  }
+
+  Position position_;
+  int started_count_;
+
+  static MockLocationProvider* instance_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockLocationProvider);
+};
+
+MockLocationProvider* MockLocationProvider::instance_ = NULL;
+
+class MockLocationObserver : public GeolocationArbitrator::Delegate {
+ public:
+  void InvalidateLastPosition() {
+    last_position_.accuracy = -1;
+    last_position_.error_code = Position::ERROR_CODE_NONE;
+    ASSERT_FALSE(last_position_.IsInitialized());
+  }
+  // Delegate
+  virtual void OnLocationUpdate(const Position& position) {
+    last_position_ = position;
+  }
+
+  Position last_position_;
+};
+
+void SetReferencePosition(Position* position) {
+  position->latitude = 51.0;
+  position->longitude = -0.1;
+  position->accuracy = 400;
+  position->timestamp = 87654321;
+  ASSERT_TRUE(position->IsInitialized());
+  ASSERT_TRUE(position->IsValidFix());
+}
+
+}  // namespace
+
+LocationProviderBase* NewMockLocationProvider() {
+  return new MockLocationProvider;
+}
+
+class GeolocationLocationArbitratorTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
+    access_token_factory_ = new FakeAccessTokenStoreFactory;
+    arbitrator_.reset(GeolocationArbitrator::New(access_token_factory_.get(),
+                                                 NULL));
+    arbitrator_->SetUseMockProvider(true);
+  }
+
+  virtual void TearDown() {
+  }
+
+  scoped_refptr<FakeAccessTokenStoreFactory> access_token_factory_;
+  scoped_ptr<GeolocationArbitrator> arbitrator_;
+};
+
+TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
+  EXPECT_TRUE(access_token_factory_);
+  EXPECT_TRUE(arbitrator_ != NULL);
+  arbitrator_.reset();
+  SUCCEED();
+}
+
+TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
+  ASSERT_TRUE(access_token_factory_);
+  ASSERT_TRUE(arbitrator_ != NULL);
+
+  EXPECT_FALSE(access_token_factory_->default_url_.is_valid());
+  EXPECT_FALSE(access_token_factory_->delegate_);
+  MockLocationObserver observer;
+  arbitrator_->AddObserver(&observer, GeolocationArbitrator::UpdateOptions());
+
+  EXPECT_TRUE(access_token_factory_->default_url_.is_valid());
+  ASSERT_TRUE(access_token_factory_->delegate_);
+
+  EXPECT_FALSE(MockLocationProvider::instance_);
+  access_token_factory_->NotifyDelegateStoreCreated();
+  ASSERT_TRUE(MockLocationProvider::instance_);
+  EXPECT_TRUE(MockLocationProvider::instance_->has_listeners());
+  EXPECT_EQ(1, MockLocationProvider::instance_->started_count_);
+  EXPECT_FALSE(observer.last_position_.IsInitialized());
+
+  SetReferencePosition(&MockLocationProvider::instance_->position_);
+  MockLocationProvider::instance_->UpdateListeners();
+  EXPECT_TRUE(observer.last_position_.IsInitialized());
+  EXPECT_EQ(MockLocationProvider::instance_->position_.latitude,
+            observer.last_position_.latitude);
+
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
+}
+
+TEST_F(GeolocationLocationArbitratorTest, MultipleListener) {
+  MockLocationObserver observer1;
+  arbitrator_->AddObserver(&observer1, GeolocationArbitrator::UpdateOptions());
+  MockLocationObserver observer2;
+  arbitrator_->AddObserver(&observer2, GeolocationArbitrator::UpdateOptions());
+
+  access_token_factory_->NotifyDelegateStoreCreated();
+  ASSERT_TRUE(MockLocationProvider::instance_);
+  EXPECT_FALSE(observer1.last_position_.IsInitialized());
+  EXPECT_FALSE(observer2.last_position_.IsInitialized());
+
+  SetReferencePosition(&MockLocationProvider::instance_->position_);
+  MockLocationProvider::instance_->UpdateListeners();
+  EXPECT_TRUE(observer1.last_position_.IsInitialized());
+  EXPECT_TRUE(observer2.last_position_.IsInitialized());
+
+  // Add third observer, and remove the first.
+  MockLocationObserver observer3;
+  arbitrator_->AddObserver(&observer3, GeolocationArbitrator::UpdateOptions());
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer1));
+  observer1.InvalidateLastPosition();
+  observer2.InvalidateLastPosition();
+  observer3.InvalidateLastPosition();
+
+  MockLocationProvider::instance_->UpdateListeners();
+  EXPECT_FALSE(observer1.last_position_.IsInitialized());
+  EXPECT_TRUE(observer2.last_position_.IsInitialized());
+  EXPECT_TRUE(observer3.last_position_.IsInitialized());
+
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer2));
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer3));
+}
+
+TEST_F(GeolocationLocationArbitratorTest, MultipleRegistrations) {
+  MockLocationObserver observer;
+  arbitrator_->AddObserver(&observer, GeolocationArbitrator::UpdateOptions());
+  GeolocationArbitrator::UpdateOptions high_accuracy_options;
+  high_accuracy_options.use_high_accuracy = true;
+  // TODO(joth): Check this causes the GPS provider to fire up.
+  arbitrator_->AddObserver(&observer, high_accuracy_options);
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
+  EXPECT_FALSE(arbitrator_->RemoveObserver(&observer));
+}
diff --git a/chrome/browser/geolocation/location_provider.h b/chrome/browser/geolocation/location_provider.h
index e5e650f..8ff4acd 100644
--- a/chrome/browser/geolocation/location_provider.h
+++ b/chrome/browser/geolocation/location_provider.h
@@ -76,9 +76,9 @@ class LocationProviderBase : public NonThreadSafe {
 
   // Inform listeners that a new position or error is available, using
   // LocationUpdateAvailable.
-  virtual void UpdateListeners();
+  void UpdateListeners();
   // Inform listeners that movement has been detected, using MovementDetected.
-  virtual void InformListenersOfMovement();
+  void InformListenersOfMovement();
 
  private:
   // The listeners registered to this provider. For each listener, we store a
diff --git a/chrome/browser/geolocation/network_location_provider.cc b/chrome/browser/geolocation/network_location_provider.cc
index 932ff0e..8d0c69e 100644
--- a/chrome/browser/geolocation/network_location_provider.cc
+++ b/chrome/browser/geolocation/network_location_provider.cc
@@ -193,7 +193,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(request_->url(), access_token);
+    access_token_store_->SetAccessToken(access_token);
   }
 
   // If new data arrived whilst request was pending reissue the request.
@@ -271,7 +271,7 @@ void NetworkLocationProvider::RequestPosition() {
   is_new_data_available_ = false;
 
   if (access_token_.empty())
-    access_token_store_->GetAccessToken(request_->url(), &access_token_);
+    access_token_store_->GetAccessToken(&access_token_);
 
   AutoLock data_lock(data_mutex_);
   request_->MakeRequest(access_token_, radio_data_, wifi_data_,
diff --git a/chrome/browser/geolocation/network_location_provider_unittest.cc b/chrome/browser/geolocation/network_location_provider_unittest.cc
index 6a56c01..f8af128 100644
--- a/chrome/browser/geolocation/network_location_provider_unittest.cc
+++ b/chrome/browser/geolocation/network_location_provider_unittest.cc
@@ -4,13 +4,11 @@
 
 #include "chrome/browser/geolocation/network_location_provider.h"
 
-#include <map>
-
 #include "base/json/json_reader.h"
 #include "base/scoped_ptr.h"
 #include "base/string_util.h"
 #include "base/values.h"
-#include "chrome/browser/geolocation/access_token_store.h"
+#include "chrome/browser/geolocation/fake_access_token_store.h"
 #include "chrome/browser/net/test_url_fetcher_factory.h"
 #include "net/url_request/url_request_status.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -46,32 +44,6 @@ class MessageLoopQuitListener
   LocationProviderBase* movement_provider_;
 };
 
-class FakeAccessTokenStore : public AccessTokenStore {
- public:
-  FakeAccessTokenStore() : allow_set_(true) {}
-
-  virtual bool SetAccessToken(const GURL& url,
-                              const string16& access_token) {
-    if (!allow_set_)
-      return false;
-    token_map_[url] = access_token;
-    return true;
-  }
-  virtual bool GetAccessToken(const GURL& url, string16* access_token) {
-    std::map<GURL, string16>::iterator item = token_map_.find(url);
-    if (item == token_map_.end())
-      return false;
-    *access_token = item->second;
-    return true;
-  }
-  bool allow_set_;
-  std::map<GURL, string16> token_map_;
-
- private:
-  ~FakeAccessTokenStore() {}
-};
-
-
 // A mock implementation of DeviceDataProviderImplBase for testing. Adapted from
 // http://gears.googlecode.com/svn/trunk/gears/geolocation/geolocation_test.cc
 template<typename DataType>
@@ -136,7 +108,7 @@ class GeolocationNetworkProviderTest : public testing::Test {
  public:
   virtual void SetUp() {
     URLFetcher::set_factory(&url_fetcher_factory_);
-    access_token_store_ = new FakeAccessTokenStore;
+    access_token_store_ = new FakeAccessTokenStore(test_server_url_);
   }
 
   virtual void TearDown() {
@@ -320,9 +292,10 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
       ResponseCookies(), kNoFixNetworkResponse);
 
   // This should have set the access token anyhow
-  EXPECT_EQ(1, static_cast<int>(access_token_store_->token_map_.size()));
+  EXPECT_EQ(access_token_store_->access_token_set_,
+            ASCIIToUTF16(REFERENCE_ACCESS_TOKEN));
   string16 token;
-  EXPECT_TRUE(access_token_store_->GetAccessToken(test_server_url_, &token));
+  EXPECT_TRUE(access_token_store_->GetAccessToken(&token));
   EXPECT_EQ(REFERENCE_ACCESS_TOKEN, UTF16ToUTF8(token));
 
   Position position;
@@ -366,8 +339,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   EXPECT_TRUE(position.IsValidFix());
 
   // Token should still be in the store.
-  EXPECT_EQ(1, static_cast<int>(access_token_store_->token_map_.size()));
-  EXPECT_TRUE(access_token_store_->GetAccessToken(test_server_url_, &token));
+  EXPECT_TRUE(access_token_store_->GetAccessToken(&token));
   EXPECT_EQ(REFERENCE_ACCESS_TOKEN, UTF16ToUTF8(token));
 
   // Wifi updated again, with one less AP. This is 'close enough' to the
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 15c0037..fc0ba76 100755
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -930,6 +930,8 @@
         'browser/geolocation/device_data_provider.h',
         'browser/geolocation/empty_device_data_provider.cc',
         'browser/geolocation/empty_device_data_provider.h',
+        'browser/geolocation/geolocation_prefs.cc',
+        'browser/geolocation/geolocation_prefs.h',
         'browser/geolocation/geoposition.cc',
         'browser/geolocation/geoposition.h',
         'browser/geolocation/location_arbitrator.cc',
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index b151ce3..a45ef70 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -707,6 +707,8 @@
         'browser/extensions/user_script_listener_unittest.cc',
         'browser/extensions/user_script_master_unittest.cc',
         'browser/find_backend_unittest.cc',
+        'browser/geolocation/fake_access_token_store.h',
+        'browser/geolocation/location_arbitrator_unittest.cc',
         'browser/geolocation/network_location_provider_unittest.cc',
         'browser/geolocation/wifi_data_provider_common_unittest.cc',
         'browser/geolocation/wifi_data_provider_unittest_win.cc',
@@ -1152,6 +1154,7 @@
         'browser/extensions/page_action_apitest.cc',
         'browser/extensions/permissions_apitest.cc',
         'browser/extensions/stubs_apitest.cc',
+        'browser/geolocation/access_token_store_browsertest.cc',
         'browser/gtk/bookmark_manager_browsertest.cc',
         'browser/net/cookie_policy_browsertest.cc',
         'browser/net/ftp_browsertest.cc',