Implement location arbitration (kudos to previous internal projects this is lifted from)

BUG=38509
TEST=GeolocationLocationArbitratorTest.Arbitration

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

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

5 files changed, 426 insertions, 88 deletions

diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc
index 0eb9a10..2776ec7 100644
--- a/chrome/browser/geolocation/location_arbitrator.cc
+++ b/chrome/browser/geolocation/location_arbitrator.cc
@@ -11,6 +11,7 @@
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/string_util.h"
+#include "base/scoped_vector.h"
 #include "chrome/browser/geolocation/access_token_store.h"
 #include "chrome/browser/geolocation/location_provider.h"
 #include "chrome/browser/profile.h"
@@ -21,23 +22,33 @@
 namespace {
 const char* kDefaultNetworkProviderUrl = "https://www.google.com/loc/json";
 GeolocationArbitrator* g_instance_ = NULL;
+// TODO(joth): Remove this global function pointer and update all tests to use
+// an injected ProviderFactory class instead.
 GeolocationArbitrator::LocationProviderFactoryFunction
     g_provider_factory_function_for_test = NULL;
 }  // namespace
 
+// To avoid oscillations, set this to twice the expected update interval of a
+// a GPS-type location provider (in case it misses a beat) plus a little.
+const int64 GeolocationArbitrator::kFixStaleTimeoutMilliseconds =
+    11 * base::Time::kMillisecondsPerSecond;
+
 class GeolocationArbitratorImpl
     : public GeolocationArbitrator,
       public LocationProviderBase::ListenerInterface,
       public NonThreadSafe {
  public:
   GeolocationArbitratorImpl(AccessTokenStore* access_token_store,
-                            URLRequestContextGetter* context_getter);
+                            URLRequestContextGetter* context_getter,
+                            GetTimeNow get_time_now,
+                            ProviderFactory* provider_factory);
   virtual ~GeolocationArbitratorImpl();
 
   // GeolocationArbitrator
   virtual void AddObserver(GeolocationArbitrator::Delegate* delegate,
                            const UpdateOptions& update_options);
   virtual bool RemoveObserver(GeolocationArbitrator::Delegate* delegate);
+  virtual Geoposition GetCurrentPosition();
   virtual void OnPermissionGranted(const GURL& requesting_frame);
   virtual bool HasPermissionBeenGranted() const;
 
@@ -49,40 +60,84 @@ class GeolocationArbitratorImpl
       AccessTokenStore::AccessTokenSet access_token_store);
 
  private:
-  void CreateProviders();
+  // Takes ownership of |provider| on entry; it will either be added to
+  // |provider_vector| or deleted on error (e.g. it fails to start).
+  void RegisterProvider(LocationProviderBase* provider,
+                        ScopedVector<LocationProviderBase>* provider_vector);
+  void ResetProviders(ScopedVector<LocationProviderBase>* providers);
+  void CreateProvidersIfRequired();
   bool HasHighAccuracyObserver();
 
+  // 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.
+  bool IsNewPositionBetter(const Geoposition& old_position,
+                           const Geoposition& new_position,
+                           bool from_same_provider) const;
+
   scoped_refptr<AccessTokenStore> access_token_store_;
   scoped_refptr<URLRequestContextGetter> context_getter_;
+  GetTimeNow get_time_now_;
+  scoped_refptr<ProviderFactory> provider_factory_;
 
-  const GURL default_url_;
-  scoped_ptr<LocationProviderBase> provider_;
+  // Low accuracy providers.
+  ScopedVector<LocationProviderBase> network_providers_;
+  // High accuracy providers.
+  ScopedVector<LocationProviderBase> gps_providers_;
 
   typedef std::map<GeolocationArbitrator::Delegate*, UpdateOptions> DelegateMap;
   DelegateMap observers_;
 
   // The current best estimate of our position.
   Geoposition position_;
+  // The provider which supplied the current |position_|
+  const LocationProviderBase* position_provider_;
 
   GURL most_recent_authorized_frame_;
 
   CancelableRequestConsumer request_consumer_;
 };
 
+class DefaultLocationProviderFactory
+    : public GeolocationArbitratorImpl::ProviderFactory {
+ public:
+  virtual LocationProviderBase* NewNetworkLocationProvider(
+      AccessTokenStore* access_token_store,
+      URLRequestContextGetter* context,
+      const GURL& url,
+      const string16& access_token) {
+    if (g_provider_factory_function_for_test)
+      return g_provider_factory_function_for_test();
+    return ::NewNetworkLocationProvider(access_token_store, context,
+                                        url, access_token);
+  }
+  virtual LocationProviderBase* NewGpsLocationProvider() {
+    if (g_provider_factory_function_for_test)
+      return NULL;
+    return ::NewGpsLocationProvider();
+  }
+};
+
 GeolocationArbitratorImpl::GeolocationArbitratorImpl(
     AccessTokenStore* access_token_store,
-    URLRequestContextGetter* context_getter)
+    URLRequestContextGetter* context_getter,
+    GetTimeNow get_time_now,
+    ProviderFactory* provider_factory)
     : access_token_store_(access_token_store),
       context_getter_(context_getter),
-      default_url_(kDefaultNetworkProviderUrl) {
-  DCHECK(default_url_.is_valid());
+      get_time_now_(get_time_now),
+      provider_factory_(provider_factory),
+      position_provider_(NULL) {
   DCHECK(NULL == g_instance_);
+  DCHECK(GURL(kDefaultNetworkProviderUrl).is_valid());
   g_instance_ = this;
 }
 
 GeolocationArbitratorImpl::~GeolocationArbitratorImpl() {
   DCHECK(CalledOnValidThread());
   DCHECK(observers_.empty()) << "Not all observers have unregistered";
+  ResetProviders(&gps_providers_);
+  ResetProviders(&network_providers_);
   DCHECK(this == g_instance_);
   g_instance_ = NULL;
 }
@@ -92,9 +147,9 @@ void GeolocationArbitratorImpl::AddObserver(
     const UpdateOptions& update_options) {
   DCHECK(CalledOnValidThread());
   observers_[delegate] = update_options;
-  if (provider_ == NULL) {
-    CreateProviders();
-  } else if (position_.IsInitialized()) {
+  CreateProvidersIfRequired();
+
+  if (position_.IsInitialized()) {
     delegate->OnLocationUpdate(position_);
   }
 }
@@ -103,20 +158,27 @@ bool GeolocationArbitratorImpl::RemoveObserver(
     GeolocationArbitrator::Delegate* delegate) {
   DCHECK(CalledOnValidThread());
   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();
-  }
+  // TODO(joth): Delayed callback to linger before destroying providers.
+  if (!HasHighAccuracyObserver())
+    ResetProviders(&gps_providers_);
+  if (observers_.empty())
+    ResetProviders(&network_providers_);
   return remove > 0;
 }
 
+Geoposition GeolocationArbitratorImpl::GetCurrentPosition() {
+  return position_;
+}
+
 void GeolocationArbitratorImpl::OnPermissionGranted(
     const GURL& requesting_frame) {
   DCHECK(CalledOnValidThread());
   most_recent_authorized_frame_ = requesting_frame;
-  if (provider_ != NULL)
-    provider_->OnPermissionGranted(requesting_frame);
+  for (ScopedVector<LocationProviderBase>::iterator i =
+          network_providers_.begin();
+      i != network_providers_.end(); ++i) {
+    (*i)->OnPermissionGranted(requesting_frame);
+  }
 }
 
 bool GeolocationArbitratorImpl::HasPermissionBeenGranted() const {
@@ -128,12 +190,17 @@ void GeolocationArbitratorImpl::LocationUpdateAvailable(
     LocationProviderBase* provider) {
   DCHECK(CalledOnValidThread());
   DCHECK(provider);
-  provider->GetPosition(&position_);
-  DCHECK(position_.IsInitialized());
-  // TODO(joth): Arbitrate.
+  Geoposition new_position;
+  provider->GetPosition(&new_position);
+  DCHECK(new_position.IsInitialized());
+  if (!IsNewPositionBetter(position_, new_position,
+                           provider == position_provider_))
+    return;
+  position_ = new_position;
+  position_provider_ = provider;
   DelegateMap::const_iterator it = observers_.begin();
   while (it != observers_.end()) {
-    // Advance iterator before callback to guard against synchronous deregister.
+    // Advance iterator before callback to guard against synchronous unregister.
     Delegate* delegate = it->first;
     ++it;
     delegate->OnLocationUpdate(position_);
@@ -147,40 +214,66 @@ void GeolocationArbitratorImpl::MovementDetected(
 
 void GeolocationArbitratorImpl::OnAccessTokenStoresLoaded(
     AccessTokenStore::AccessTokenSet access_token_set) {
-  DCHECK(provider_ == NULL)
+  DCHECK(network_providers_.empty())
       << "OnAccessTokenStoresLoaded : has existing location "
       << "provider. Race condition caused repeat load of tokens?";
-  if (g_provider_factory_function_for_test) {
-    provider_.reset(g_provider_factory_function_for_test());
-  } else {
-    provider_.reset(NewGpsLocationProvider());
-    if (provider_ == NULL) {
-      // TODO(joth): When arbitration is implemented, iterate the whole
-      // set rather than cherry-pick our default url.
-      provider_.reset(NewNetworkLocationProvider(
-          access_token_store_.get(), context_getter_.get(), default_url_,
-          access_token_set[default_url_]));
-    }
+  // If there are no access tokens, boot strap it with the default server URL.
+  if (access_token_set.empty())
+    access_token_set[GURL(kDefaultNetworkProviderUrl)];
+  for (AccessTokenStore::AccessTokenSet::iterator i =
+           access_token_set.begin();
+      i != access_token_set.end(); ++i) {
+    RegisterProvider(
+        provider_factory_->NewNetworkLocationProvider(
+            access_token_store_.get(), context_getter_.get(),
+            i->first, i->second),
+        &network_providers_);
+  }
+}
+
+void GeolocationArbitratorImpl::RegisterProvider(
+    LocationProviderBase* provider,
+    ScopedVector<LocationProviderBase>* provider_vector) {
+  DCHECK(provider_vector);
+  if (!provider)
+    return;
+  provider->RegisterListener(this);
+  if (!provider->StartProvider()) {
+    LOG(WARNING) << "Failed to start provider " << provider;
+    delete provider;
+    return;
   }
-  DCHECK(provider_ != NULL);
-  provider_->RegisterListener(this);
-  const bool ok = provider_->StartProvider();
-  DCHECK(ok);
   if (most_recent_authorized_frame_.is_valid())
-    provider_->OnPermissionGranted(most_recent_authorized_frame_);
+    provider->OnPermissionGranted(most_recent_authorized_frame_);
+  provider_vector->push_back(provider);
 }
 
-void GeolocationArbitratorImpl::CreateProviders() {
+void GeolocationArbitratorImpl::ResetProviders(
+    ScopedVector<LocationProviderBase>* providers) {
+  DCHECK(providers);
+  if (providers->empty())
+    return;
+  for (ScopedVector<LocationProviderBase>::iterator i = providers->begin();
+      i != providers->end(); ++i) {
+    (*i)->UnregisterListener(this);
+  }
+  providers->reset();
+}
+
+void GeolocationArbitratorImpl::CreateProvidersIfRequired() {
   DCHECK(CalledOnValidThread());
   DCHECK(!observers_.empty());
-  DCHECK(provider_ == NULL);
-  if (!request_consumer_.HasPendingRequests()) {
+  if (network_providers_.empty() && !request_consumer_.HasPendingRequests()) {
+    // There are no network providers either created or pending creation.
     access_token_store_->LoadAccessTokens(
         &request_consumer_,
         NewCallback(this,
                     &GeolocationArbitratorImpl::OnAccessTokenStoresLoaded));
   }
-  // TODO(joth): Use high accuracy flag to conditionally create GPS provider.
+  if (gps_providers_.empty() && HasHighAccuracyObserver()) {
+    RegisterProvider(provider_factory_->NewGpsLocationProvider(),
+                     &gps_providers_);
+  }
 }
 
 bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
@@ -193,11 +286,39 @@ bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
   return false;
 }
 
+bool GeolocationArbitratorImpl::IsNewPositionBetter(
+    const Geoposition& old_position, const Geoposition& new_position,
+    bool from_same_provider) const {
+  // Updates location_info if it's better than what we currently have,
+  // or if it's a newer update from the same provider.
+  if (!old_position.IsValidFix()) {
+      // Older location wasn't locked.
+      return true;
+    }
+  if (new_position.IsValidFix()) {
+    // New location is locked, let's check if it's any better.
+    if (old_position.accuracy >= new_position.accuracy) {
+      // Accuracy is better.
+      return true;
+    } else if (from_same_provider) {
+      // Same provider, fresher location.
+      return true;
+    } else if ((get_time_now_() - old_position.timestamp).InMilliseconds() >
+               kFixStaleTimeoutMilliseconds) {
+      // Existing fix is stale.
+      return true;
+    }
+  }
+  return false;
+}
+
 GeolocationArbitrator* GeolocationArbitrator::Create(
     AccessTokenStore* access_token_store,
-    URLRequestContextGetter* context_getter) {
-  DCHECK(!g_instance_);
-  return new GeolocationArbitratorImpl(access_token_store, context_getter);
+    URLRequestContextGetter* context_getter,
+    GetTimeNow get_time_now,
+    ProviderFactory* provider_factory) {
+  return new GeolocationArbitratorImpl(access_token_store, context_getter,
+                                       get_time_now, provider_factory);
 }
 
 GeolocationArbitrator* GeolocationArbitrator::GetInstance() {
@@ -207,7 +328,9 @@ GeolocationArbitrator* GeolocationArbitrator::GetInstance() {
     // particularly important which profile it is attached to: the network
     // request implementation disables cookies anyhow.
     Create(NewChromePrefsAccessTokenStore(),
-           Profile::GetDefaultRequestContext());
+           Profile::GetDefaultRequestContext(),
+           base::Time::Now,
+           new DefaultLocationProviderFactory);
     DCHECK(g_instance_);
   }
   return g_instance_;
@@ -223,3 +346,6 @@ GeolocationArbitrator::GeolocationArbitrator() {
 
 GeolocationArbitrator::~GeolocationArbitrator() {
 }
+
+GeolocationArbitrator::ProviderFactory::~ProviderFactory() {
+}
diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h
index 4742e56..085e320 100644
--- a/chrome/browser/geolocation/location_arbitrator.h
+++ b/chrome/browser/geolocation/location_arbitrator.h
@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_
 #define CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_
 
+#include "base/string16.h"
+#include "base/time.h"
 #include "base/ref_counted.h"
 
 class AccessTokenStore;
@@ -23,11 +25,37 @@ struct Geoposition;
 // moment.
 class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> {
  public:
+  // Number of milliseconds newer a location provider has to be that it's worth
+  // switching to this location provider on the basis of it being fresher
+  // (regardles of relative accuracy). Public for tests.
+  static const int64 kFixStaleTimeoutMilliseconds;
+
+  // Defines a function that returns the current time.
+  typedef base::Time (*GetTimeNow)();
+
+  // Allows injection of factory methods for creating the location providers.
+  // RefCounted for simplicity of writing tests.
+  class ProviderFactory  : public base::RefCounted<ProviderFactory> {
+   public:
+    virtual LocationProviderBase* NewNetworkLocationProvider(
+        AccessTokenStore* access_token_store,
+        URLRequestContextGetter* context,
+        const GURL& url,
+        const string16& access_token) = 0;
+    virtual LocationProviderBase* NewGpsLocationProvider() = 0;
+
+   protected:
+    friend class base::RefCounted<ProviderFactory>;
+    virtual ~ProviderFactory();
+  };
+
   // Creates and returns a new instance of the location arbitrator. Allows
-  // injection of the access token store and url getter context, for testing.
+  // injection of dependencies, for testing.
   static GeolocationArbitrator* Create(
       AccessTokenStore* access_token_store,
-      URLRequestContextGetter* context_getter);
+      URLRequestContextGetter* context_getter,
+      GetTimeNow get_time_now,
+      ProviderFactory* provider_factory);
 
   // Gets a pointer to the singleton instance of the location arbitrator, which
   // is in turn bound to the browser's global context objects. Ownership is NOT
@@ -55,7 +83,7 @@ class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> {
   // The update options passed are used as a 'hint' for the provider preferences
   // for this particular observer, however the delegate could receive callbacks
   // for best available locations from any active provider whilst it is
-  // registerd.
+  // registered.
   // If an existing delegate is added a second time it's options are updated
   // but only a single call to RemoveObserver() is required to remove it.
   virtual void AddObserver(Delegate* delegate,
@@ -64,6 +92,11 @@ class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> {
   // via AddObserver(). Returns true if the observer was removed.
   virtual bool RemoveObserver(Delegate* delegate) = 0;
 
+  // Returns the current position estimate, or an uninitialized position
+  // if none is yet available. Once initialized, this will always match
+  // the most recent observer notification (via Delegate::OnLocationUpdate()).
+  virtual Geoposition GetCurrentPosition() = 0;
+
   // Called everytime permission is granted to a page for using geolocation.
   // This may either be through explicit user action (e.g. responding to the
   // infobar prompt) or inferred from a persisted site permission.
@@ -75,8 +108,10 @@ class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> {
   // OnPermissionGranted().
   virtual bool HasPermissionBeenGranted() const = 0;
 
-  // For testing, a factory functino can be set which will be used to create
+  // For testing, a factory function can be set which will be used to create
   // a specified test provider. Pass NULL to reset to the default behavior.
+  // For finer grained control, use class ProviderFactory instead.
+  // TODO(joth): Move all tests to use ProviderFactory and remove this.
   typedef LocationProviderBase* (*LocationProviderFactoryFunction)(void);
   static void SetProviderFactoryForTest(
       LocationProviderFactoryFunction factory_function);
diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc
index 38dd9da..be29d50 100644
--- a/chrome/browser/geolocation/location_arbitrator_unittest.cc
+++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc
@@ -28,30 +28,89 @@ class MockLocationObserver : public GeolocationArbitrator::Delegate {
   Geoposition last_position_;
 };
 
-void SetReferencePosition(Geoposition* position) {
-  position->latitude = 51.0;
-  position->longitude = -0.1;
-  position->accuracy = 400;
-  position->timestamp = base::Time::FromDoubleT(87654321.0);
+class MockProviderFactory : public GeolocationArbitrator::ProviderFactory {
+ public:
+  MockProviderFactory()
+      : cell_(NULL), gps_(NULL) {
+  }
+  virtual LocationProviderBase* NewNetworkLocationProvider(
+      AccessTokenStore* access_token_store,
+      URLRequestContextGetter* context,
+      const GURL& url,
+      const string16& access_token) {
+    return new MockLocationProvider(&cell_);
+  }
+  virtual LocationProviderBase* NewGpsLocationProvider() {
+    return new MockLocationProvider(&gps_);
+  }
+
+  // Two location providers, with nice short names to make the tests more
+  // readable. Note |gps_| will only be set when there is a high accuracy
+  // observer registered (and |cell_| when there's at least one observer of any
+  // type).
+  MockLocationProvider* cell_;
+  MockLocationProvider* gps_;
+};
+
+void SetPositionFix(Geoposition* position,
+                    double latitude,
+                    double longitude,
+                    double accuracy,
+                    const base::Time& timestamp) {
+  ASSERT_TRUE(position);
+  position->error_code = Geoposition::ERROR_CODE_NONE;
+  position->latitude = latitude;
+  position->longitude = longitude;
+  position->accuracy = accuracy;
+  position->timestamp = timestamp;
   ASSERT_TRUE(position->IsInitialized());
   ASSERT_TRUE(position->IsValidFix());
 }
 
-}  // namespace
+void SetReferencePosition(Geoposition* position) {
+  SetPositionFix(position, 51.0, -0.1, 400, base::Time::FromDoubleT(54321.0));
+}
+
+double g_fake_time_now_secs = 1;
+
+base::Time GetTimeNow() {
+  return base::Time::FromDoubleT(g_fake_time_now_secs);
+}
+
+void AdvanceTimeNow(const base::TimeDelta& delta) {
+  g_fake_time_now_secs += delta.InSecondsF();
+}
 
 class GeolocationLocationArbitratorTest : public testing::Test {
  protected:
   virtual void SetUp() {
     access_token_store_ = new FakeAccessTokenStore;
-    GeolocationArbitrator::SetProviderFactoryForTest(&NewMockLocationProvider);
+    providers_ = new MockProviderFactory();
     arbitrator_ = GeolocationArbitrator::Create(access_token_store_.get(),
-                                                NULL);
+                                                NULL, GetTimeNow, providers_);
   }
 
   virtual void TearDown() {
   }
 
+  void CheckLastPositionInfo(double latitude,
+                             double longitude,
+                             double accuracy) {
+    Geoposition geoposition = arbitrator_->GetCurrentPosition();
+    EXPECT_TRUE(geoposition.IsValidFix());
+    EXPECT_DOUBLE_EQ(latitude, geoposition.latitude);
+    EXPECT_DOUBLE_EQ(longitude, geoposition.longitude);
+    EXPECT_DOUBLE_EQ(accuracy, geoposition.accuracy);
+  }
+
+  base::TimeDelta SwitchOnFreshnessCliff() {
+    // Add 1, to ensure it meets any greater-than test.
+    return base::TimeDelta::FromMilliseconds(
+        GeolocationArbitrator::kFixStaleTimeoutMilliseconds + 1);
+  }
+
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
+  scoped_refptr<MockProviderFactory> providers_;
   scoped_refptr<GeolocationArbitrator> arbitrator_;
 };
 
@@ -68,7 +127,8 @@ TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
   // Can't check the provider has been notified without going through the
   // motions to create the provider (see next test).
-  EXPECT_FALSE(MockLocationProvider::instance_);
+  EXPECT_FALSE(providers_->cell_);
+  EXPECT_FALSE(providers_->gps_);
 }
 
 TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
@@ -83,29 +143,31 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   EXPECT_TRUE(access_token_store_->access_token_set_.empty());
   ASSERT_TRUE(access_token_store_->request_);
 
-  EXPECT_FALSE(MockLocationProvider::instance_);
+  EXPECT_FALSE(providers_->cell_);
+  EXPECT_FALSE(providers_->gps_);
   access_token_store_->NotifyDelegateTokensLoaded();
-  ASSERT_TRUE(MockLocationProvider::instance_);
-  EXPECT_TRUE(MockLocationProvider::instance_->has_listeners());
-  EXPECT_EQ(1, MockLocationProvider::instance_->started_count_);
+  ASSERT_TRUE(providers_->cell_);
+  EXPECT_FALSE(providers_->gps_);
+  EXPECT_TRUE(providers_->cell_->has_listeners());
+  EXPECT_EQ(1, providers_->cell_->started_count_);
   EXPECT_FALSE(observer.last_position_.IsInitialized());
 
-  SetReferencePosition(&MockLocationProvider::instance_->position_);
-  MockLocationProvider::instance_->UpdateListeners();
+  SetReferencePosition(&providers_->cell_->position_);
+  providers_->cell_->UpdateListeners();
   EXPECT_TRUE(observer.last_position_.IsInitialized());
-  EXPECT_EQ(MockLocationProvider::instance_->position_.latitude,
+  EXPECT_EQ(providers_->cell_->position_.latitude,
             observer.last_position_.latitude);
 
   EXPECT_FALSE(
-      MockLocationProvider::instance_->permission_granted_url_.is_valid());
+      providers_->cell_->permission_granted_url_.is_valid());
   EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
   GURL frame_url("http://frame.test");
   arbitrator_->OnPermissionGranted(frame_url);
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
   EXPECT_TRUE(
-      MockLocationProvider::instance_->permission_granted_url_.is_valid());
+      providers_->cell_->permission_granted_url_.is_valid());
   EXPECT_EQ(frame_url,
-            MockLocationProvider::instance_->permission_granted_url_);
+            providers_->cell_->permission_granted_url_);
 
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
 }
@@ -117,12 +179,12 @@ TEST_F(GeolocationLocationArbitratorTest, MultipleListener) {
   arbitrator_->AddObserver(&observer2, GeolocationArbitrator::UpdateOptions());
 
   access_token_store_->NotifyDelegateTokensLoaded();
-  ASSERT_TRUE(MockLocationProvider::instance_);
+  ASSERT_TRUE(providers_->cell_);
   EXPECT_FALSE(observer1.last_position_.IsInitialized());
   EXPECT_FALSE(observer2.last_position_.IsInitialized());
 
-  SetReferencePosition(&MockLocationProvider::instance_->position_);
-  MockLocationProvider::instance_->UpdateListeners();
+  SetReferencePosition(&providers_->cell_->position_);
+  providers_->cell_->UpdateListeners();
   EXPECT_TRUE(observer1.last_position_.IsInitialized());
   EXPECT_TRUE(observer2.last_position_.IsInitialized());
 
@@ -134,7 +196,7 @@ TEST_F(GeolocationLocationArbitratorTest, MultipleListener) {
   observer2.InvalidateLastPosition();
   observer3.InvalidateLastPosition();
 
-  MockLocationProvider::instance_->UpdateListeners();
+  providers_->cell_->UpdateListeners();
   EXPECT_FALSE(observer1.last_position_.IsInitialized());
   EXPECT_TRUE(observer2.last_position_.IsInitialized());
   EXPECT_TRUE(observer3.last_position_.IsInitialized());
@@ -148,9 +210,10 @@ TEST_F(GeolocationLocationArbitratorTest,
   MockLocationObserver observer;
   arbitrator_->AddObserver(
       &observer, GeolocationArbitrator::UpdateOptions(false));
-  // TODO(joth): Check this causes the GPS provider to fire up.
+  EXPECT_FALSE(providers_->gps_);
   arbitrator_->AddObserver(
       &observer, GeolocationArbitrator::UpdateOptions(true));
+  EXPECT_TRUE(providers_->gps_);
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
   EXPECT_FALSE(arbitrator_->RemoveObserver(&observer));
 }
@@ -160,10 +223,10 @@ TEST_F(GeolocationLocationArbitratorTest, RegistrationAfterFixArrives) {
   arbitrator_->AddObserver(&observer1, GeolocationArbitrator::UpdateOptions());
 
   access_token_store_->NotifyDelegateTokensLoaded();
-  ASSERT_TRUE(MockLocationProvider::instance_);
+  ASSERT_TRUE(providers_->cell_);
   EXPECT_FALSE(observer1.last_position_.IsInitialized());
-  SetReferencePosition(&MockLocationProvider::instance_->position_);
-  MockLocationProvider::instance_->UpdateListeners();
+  SetReferencePosition(&providers_->cell_->position_);
+  providers_->cell_->UpdateListeners();
   EXPECT_TRUE(observer1.last_position_.IsValidFix());
 
   MockLocationObserver observer2;
@@ -174,3 +237,111 @@ TEST_F(GeolocationLocationArbitratorTest, RegistrationAfterFixArrives) {
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer1));
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer2));
 }
+
+
+TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
+  // No position so far
+  EXPECT_FALSE(arbitrator_->GetCurrentPosition().IsInitialized());
+  MockLocationObserver observer;
+  arbitrator_->AddObserver(&observer,
+                           GeolocationArbitrator::UpdateOptions(true));
+  access_token_store_->NotifyDelegateTokensLoaded();
+  ASSERT_TRUE(providers_->cell_);
+  ASSERT_TRUE(providers_->gps_);
+
+  SetPositionFix(&providers_->cell_->position_, 1, 2, 150, GetTimeNow());
+  providers_->cell_->UpdateListeners();
+
+  // First position available
+  EXPECT_TRUE(arbitrator_->GetCurrentPosition().IsValidFix());
+  CheckLastPositionInfo(1, 2, 150);
+
+  SetPositionFix(&providers_->gps_->position_, 3, 4, 50, GetTimeNow());
+  providers_->gps_->UpdateListeners();
+
+  // More accurate fix available
+  CheckLastPositionInfo(3, 4, 50);
+
+  SetPositionFix(&providers_->cell_->position_, 5, 6, 150, GetTimeNow());
+  providers_->cell_->UpdateListeners();
+
+  // New fix is available but it's less accurate, older fix should be kept.
+  CheckLastPositionInfo(3, 4, 50);
+
+  // Advance time, and notify once again
+  AdvanceTimeNow(SwitchOnFreshnessCliff());
+  providers_->cell_->UpdateListeners();
+
+  // New fix is available, less accurate but fresher
+  CheckLastPositionInfo(5, 6, 150);
+
+  // Advance time, and set a low accuracy position
+  AdvanceTimeNow(SwitchOnFreshnessCliff());
+  SetPositionFix(&providers_->cell_->position_, 5.676731, 139.629385, 1000,
+                 GetTimeNow());
+  providers_->cell_->UpdateListeners();
+  CheckLastPositionInfo(5.676731, 139.629385, 1000);
+
+  // 15 secs later, step outside. Switches to gps signal.
+  AdvanceTimeNow(base::TimeDelta::FromSeconds(15));
+  SetPositionFix(&providers_->gps_->position_, 3.5676457, 139.629198, 50,
+                 GetTimeNow());
+  providers_->gps_->UpdateListeners();
+  CheckLastPositionInfo(3.5676457, 139.629198, 50);
+
+  // 5 mins later switch cells while walking. Stay on gps.
+  AdvanceTimeNow(base::TimeDelta::FromMinutes(5));
+  SetPositionFix(&providers_->cell_->position_, 3.567832, 139.634648, 300,
+                 GetTimeNow());
+  SetPositionFix(&providers_->gps_->position_, 3.5677675, 139.632314, 50,
+                 GetTimeNow());
+  providers_->cell_->UpdateListeners();
+  providers_->gps_->UpdateListeners();
+  CheckLastPositionInfo(3.5677675, 139.632314, 50);
+
+  // Ride train and gps signal degrades slightly. Stay on fresher gps
+  AdvanceTimeNow(base::TimeDelta::FromMinutes(5));
+  SetPositionFix(&providers_->gps_->position_, 3.5679026, 139.634777, 300,
+                 GetTimeNow());
+  providers_->gps_->UpdateListeners();
+  CheckLastPositionInfo(3.5679026, 139.634777, 300);
+
+  // 14 minutes later
+  AdvanceTimeNow(base::TimeDelta::FromMinutes(14));
+
+  // GPS reading misses a beat, but don't switch to cell yet to avoid
+  // oscillating.
+  SetPositionFix(&providers_->gps_->position_, 3.5659005, 139.682579, 300,
+                 GetTimeNow());
+  providers_->gps_->UpdateListeners();
+
+  AdvanceTimeNow(base::TimeDelta::FromSeconds(7));
+  SetPositionFix(&providers_->cell_->position_, 3.5689579, 139.691420, 1000,
+                 GetTimeNow());
+  providers_->cell_->UpdateListeners();
+  CheckLastPositionInfo(3.5659005, 139.682579, 300);
+
+  // 1 minute later
+  AdvanceTimeNow(base::TimeDelta::FromMinutes(1));
+
+  // Enter tunnel. Stay on fresher gps for a moment.
+  SetPositionFix(&providers_->cell_->position_, 3.5657078, 139.68922, 300,
+                 GetTimeNow());
+  providers_->cell_->UpdateListeners();
+  SetPositionFix(&providers_->gps_->position_, 3.5657104, 139.690341, 300,
+                 GetTimeNow());
+  providers_->gps_->UpdateListeners();
+  CheckLastPositionInfo(3.5657104, 139.690341, 300);
+
+  // 2 minutes later
+  AdvanceTimeNow(base::TimeDelta::FromMinutes(2));
+  // Arrive in station. Cell moves but GPS is stale. Switch to fresher cell.
+  SetPositionFix(&providers_->cell_->position_, 3.5658700, 139.069979, 1000,
+                 GetTimeNow());
+  providers_->cell_->UpdateListeners();
+  CheckLastPositionInfo(3.5658700, 139.069979, 1000);
+
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
+}
+
+}  // namespace
diff --git a/chrome/browser/geolocation/mock_location_provider.cc b/chrome/browser/geolocation/mock_location_provider.cc
index 7a7bb9c..8543848 100644
--- a/chrome/browser/geolocation/mock_location_provider.cc
+++ b/chrome/browser/geolocation/mock_location_provider.cc
@@ -14,15 +14,17 @@
 
 MockLocationProvider* MockLocationProvider::instance_ = NULL;
 
-MockLocationProvider::MockLocationProvider()
-    : started_count_(0) {
-  CHECK(instance_ == NULL);
-  instance_ = this;
+MockLocationProvider::MockLocationProvider(MockLocationProvider** self_ref)
+    : started_count_(0),
+      self_ref_(self_ref) {
+  CHECK(self_ref_);
+  CHECK(*self_ref_ == NULL);
+  *self_ref_ = this;
 }
 
 MockLocationProvider::~MockLocationProvider() {
-  CHECK(instance_ == this);
-  instance_ = NULL;
+  CHECK(*self_ref_ == this);
+  *self_ref_ = NULL;
 }
 
 bool MockLocationProvider::StartProvider() {
@@ -44,7 +46,8 @@ class AutoMockLocationProvider : public MockLocationProvider {
  public:
   AutoMockLocationProvider(bool has_valid_location,
                            bool requires_permission_to_start)
-      : ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)),
+      : MockLocationProvider(&instance_),
+        ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)),
         requires_permission_to_start_(requires_permission_to_start) {
     if (has_valid_location) {
       position_.accuracy = 3;
@@ -79,7 +82,7 @@ class AutoMockLocationProvider : public MockLocationProvider {
 };
 
 LocationProviderBase* NewMockLocationProvider() {
-  return new MockLocationProvider;
+  return new MockLocationProvider(&MockLocationProvider::instance_);
 }
 
 LocationProviderBase* NewAutoSuccessMockLocationProvider() {
diff --git a/chrome/browser/geolocation/mock_location_provider.h b/chrome/browser/geolocation/mock_location_provider.h
index 50b454b..99ee378 100644
--- a/chrome/browser/geolocation/mock_location_provider.h
+++ b/chrome/browser/geolocation/mock_location_provider.h
@@ -11,8 +11,10 @@
 // Mock implementation of a location provider for testing.
 class MockLocationProvider : public LocationProviderBase {
  public:
-  MockLocationProvider();
-  virtual ~MockLocationProvider();
+  // Will update |*self_ref| to point to |this| on construction, and to NULL
+  // on destruction.
+  explicit MockLocationProvider(MockLocationProvider** self_ref);
+  ~MockLocationProvider();
 
   using LocationProviderBase::UpdateListeners;
   using LocationProviderBase::InformListenersOfMovement;
@@ -25,6 +27,7 @@ class MockLocationProvider : public LocationProviderBase {
   Geoposition position_;
   int started_count_;
   GURL permission_granted_url_;
+  MockLocationProvider** self_ref_;
 
   // Set when an instance of the mock is created via a factory function.
   static MockLocationProvider* instance_;
@@ -35,7 +38,7 @@ class MockLocationProvider : public LocationProviderBase {
 // Factory functions for the various sorts of mock location providers,
 // for use with GeolocationArbitrator::SetProviderFactoryForTest (i.e.
 // not intended for test code to use to get access to the mock, you can use
-// MockLocationProvider::instance_ for this, or make a custom facotry method).
+// MockLocationProvider::instance_ for this, or make a custom factory method).
 
 // Creates a mock location provider with no default behavior.
 LocationProviderBase* NewMockLocationProvider();