Gets rid of the LocationArbitrator interface, in preference for LocationProvider.

This CL reorganizes classes that derived from LocationArbitrator to instead
derive from LocationProvider. There was a quite a bit of overlap between the
two interfaces, and merging the two in favor of LocationProvider makes later
work with Blimp much easier.

Other work included:
*Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest

BUG=614486, 637869

Review-Url: https://codereview.chromium.org/2226143002
Cr-Commit-Position: refs/heads/master@{#414735}

blimp/engine/feature/geolocation/blimp_location_provider.cc

@@ -44,21 +44,17 @@ void BlimpLocationProvider::StopProvider() {
   }
 }
 
-void BlimpLocationProvider::GetPosition(device::Geoposition* position) {
-  *position = cached_position_;
+const device::Geoposition& BlimpLocationProvider::GetPosition() {
+  return cached_position_;
 }
 
-void BlimpLocationProvider::RequestRefresh() {
+void BlimpLocationProvider::OnPermissionGranted() {
   DCHECK(is_started_);
   if (delegate_) {
     delegate_->RequestRefresh();
   }
 }
 
-void BlimpLocationProvider::OnPermissionGranted() {
-  RequestRefresh();
-}
-
 void BlimpLocationProvider::SetUpdateCallback(
     const LocationProviderUpdateCallback& callback) {
   if (delegate_) {

blimp/engine/feature/geolocation/blimp_location_provider.h

@@ -37,8 +37,7 @@ class BlimpLocationProvider : public device::LocationProvider {
   // device::LocationProvider implementation.
   bool StartProvider(bool high_accuracy) override;
   void StopProvider() override;
-  void GetPosition(device::Geoposition* position) override;
-  void RequestRefresh() override;
+  const device::Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;

blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc

@@ -106,22 +106,7 @@ TEST_F(BlimpLocationProviderTest, LocationProviderDeleted) {
   location_provider_.reset();
 }
 
-TEST_F(BlimpLocationProviderTest, RequestRefreshRunsCorrectly) {
-  EXPECT_CALL(*delegate_, RequestRefresh()).Times(1);
-
-  location_provider_->StartProvider(true);
-  location_provider_->RequestRefresh();
-}
-
-TEST_F(BlimpLocationProviderTest, RequestRefreshHandlesNullDelegate) {
-  EXPECT_CALL(*delegate_, RequestRefresh()).Times(0);
-
-  location_provider_->StartProvider(true);
-  delegate_.reset();
-  location_provider_->RequestRefresh();
-}
-
-TEST_F(BlimpLocationProviderTest, OnPermissionGrantedCallsRefresh) {
+TEST_F(BlimpLocationProviderTest, OnPermissionGranted) {
   EXPECT_CALL(*delegate_, RequestRefresh()).Times(1);
 
   location_provider_->StartProvider(true);

blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc

@@ -180,7 +180,8 @@ TEST_F(EngineGeolocationFeatureTest, UnexpectedMessageReceived) {
   EXPECT_EQ(net::ERR_UNEXPECTED, cb.WaitForResult());
 }
 
-TEST_F(EngineGeolocationFeatureTest, RequestRefresh) {
+TEST_F(EngineGeolocationFeatureTest,
+       OnPermissionGrantedTriggersRefreshRequest) {
   EXPECT_CALL(*out_processor_,
               MockableProcessMessage(
                   EqualsUpdatedRequestLevel(
@@ -199,7 +200,7 @@ TEST_F(EngineGeolocationFeatureTest, RequestRefresh) {
       .Times(1);
 
   location_provider_->StartProvider(true);
-  location_provider_->RequestRefresh();
+  location_provider_->OnPermissionGranted();
 }
 
 }  // namespace engine

device/geolocation/BUILD.gn

@@ -31,7 +31,6 @@ component("device_geolocation") {
     "geoposition.h",
     "location_api_adapter_android.cc",
     "location_api_adapter_android.h",
-    "location_arbitrator.h",
     "location_arbitrator_impl.cc",
     "location_arbitrator_impl.h",
     "location_provider.h",
@@ -155,8 +154,6 @@ source_set("unittests") {
     "fake_access_token_store.h",
     "geolocation_provider_impl_unittest.cc",
     "location_arbitrator_impl_unittest.cc",
-    "mock_location_arbitrator.cc",
-    "mock_location_arbitrator.h",
     "mock_location_provider.cc",
     "mock_location_provider.h",
     "network_location_provider_unittest.cc",

device/geolocation/geolocation_provider_impl.cc

@@ -4,6 +4,8 @@
 
 #include "device/geolocation/geolocation_provider_impl.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -72,7 +74,8 @@ void GeolocationProviderImpl::OverrideLocationForTesting(
   NotifyClients(position);
 }
 
-void GeolocationProviderImpl::OnLocationUpdate(const Geoposition& position) {
+void GeolocationProviderImpl::OnLocationUpdate(const LocationProvider* provider,
+                                               const Geoposition& position) {
   DCHECK(OnGeolocationThread());
   // Will be true only in testing.
   if (ignore_location_updates_)
@@ -104,6 +107,11 @@ GeolocationProviderImpl::~GeolocationProviderImpl() {
   DCHECK(!arbitrator_);
 }
 
+void GeolocationProviderImpl::SetArbitratorForTesting(
+    std::unique_ptr<LocationProvider> arbitrator) {
+  arbitrator_ = std::move(arbitrator);
+}
+
 bool GeolocationProviderImpl::OnGeolocationThread() const {
   return task_runner()->BelongsToCurrentThread();
 }
@@ -140,13 +148,13 @@ void GeolocationProviderImpl::OnClientsChanged() {
 void GeolocationProviderImpl::StopProviders() {
   DCHECK(OnGeolocationThread());
   DCHECK(arbitrator_);
-  arbitrator_->StopProviders();
+  arbitrator_->StopProvider();
 }
 
 void GeolocationProviderImpl::StartProviders(bool enable_high_accuracy) {
   DCHECK(OnGeolocationThread());
   DCHECK(arbitrator_);
-  arbitrator_->StartProviders(enable_high_accuracy);
+  arbitrator_->StartProvider(enable_high_accuracy);
 }
 
 void GeolocationProviderImpl::InformProvidersPermissionGranted() {
@@ -174,25 +182,23 @@ void GeolocationProviderImpl::NotifyClients(const Geoposition& position) {
 
 void GeolocationProviderImpl::Init() {
   DCHECK(OnGeolocationThread());
-  DCHECK(!arbitrator_);
-  arbitrator_ = CreateArbitrator();
-}
 
-void GeolocationProviderImpl::CleanUp() {
-  DCHECK(OnGeolocationThread());
-  arbitrator_.reset();
-}
-
-std::unique_ptr<LocationArbitrator>
-GeolocationProviderImpl::CreateArbitrator() {
-  LocationArbitratorImpl::LocationUpdateCallback callback = base::Bind(
+  if (!arbitrator_) {
+    LocationProvider::LocationProviderUpdateCallback callback = base::Bind(
         &GeolocationProviderImpl::OnLocationUpdate, base::Unretained(this));
     // Use the embedder's |g_delegate| or fall back to the default one.
     if (!g_delegate.Get())
       g_delegate.Get().reset(new GeolocationDelegate);
 
-  return base::MakeUnique<LocationArbitratorImpl>(callback,
-                                                  g_delegate.Get().get());
+    arbitrator_ =
+        base::MakeUnique<LocationArbitratorImpl>(g_delegate.Get().get());
+    arbitrator_->SetUpdateCallback(callback);
+  }
+}
+
+void GeolocationProviderImpl::CleanUp() {
+  DCHECK(OnGeolocationThread());
+  arbitrator_.reset();
 }
 
 }  // namespace device

device/geolocation/geolocation_provider_impl.h

@@ -16,6 +16,7 @@
 #include "device/geolocation/geolocation_export.h"
 #include "device/geolocation/geolocation_provider.h"
 #include "device/geolocation/geoposition.h"
+#include "device/geolocation/location_provider.h"
 
 namespace base {
 template <typename Type>
@@ -24,7 +25,6 @@ class SingleThreadTaskRunner;
 }
 
 namespace device {
-class LocationArbitrator;
 
 class DEVICE_GEOLOCATION_EXPORT GeolocationProviderImpl
     : public NON_EXPORTED_BASE(GeolocationProvider),
@@ -38,7 +38,8 @@ class DEVICE_GEOLOCATION_EXPORT GeolocationProviderImpl
   void OverrideLocationForTesting(const Geoposition& position) override;
 
   // Callback from the LocationArbitrator. Public for testing.
-  void OnLocationUpdate(const Geoposition& position);
+  void OnLocationUpdate(const LocationProvider* provider,
+                        const Geoposition& position);
 
   // Gets a pointer to the singleton instance of the location relayer, which
   // is in turn bound to the browser's global context objects. This must only be
@@ -50,16 +51,15 @@ class DEVICE_GEOLOCATION_EXPORT GeolocationProviderImpl
     return user_did_opt_into_location_services_;
   }
 
- protected:
+  // Safe to call while there are no GeolocationProviderImpl clients
+  // registered.
+  void SetArbitratorForTesting(std::unique_ptr<LocationProvider> arbitrator);
+
+ private:
   friend struct base::DefaultSingletonTraits<GeolocationProviderImpl>;
   GeolocationProviderImpl();
   ~GeolocationProviderImpl() override;
 
-  // Useful for injecting mock geolocation arbitrator in tests.
-  // TODO(mvanouwerkerk): Use something like SetArbitratorForTesting instead.
-  virtual std::unique_ptr<LocationArbitrator> CreateArbitrator();
-
- private:
   bool OnGeolocationThread() const;
 
   // Start and stop providers as needed when clients are added or removed.
@@ -93,12 +93,12 @@ class DEVICE_GEOLOCATION_EXPORT GeolocationProviderImpl
   // True only in testing, where we want to use a custom position.
   bool ignore_location_updates_;
 
-  // Only to be used on the geolocation thread.
-  std::unique_ptr<LocationArbitrator> arbitrator_;
-
-  // Used to PostTask()s from the geolocation thread to creation thread.
+  // Used to PostTask()s from the geolocation thread to caller thread.
   const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 
+  // Only to be used on the geolocation thread.
+  std::unique_ptr<LocationProvider> arbitrator_;
+
   DISALLOW_COPY_AND_ASSIGN(GeolocationProviderImpl);
 };
 

device/geolocation/geolocation_provider_impl_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
@@ -17,7 +18,7 @@
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "device/geolocation/access_token_store.h"
-#include "device/geolocation/mock_location_arbitrator.h"
+#include "device/geolocation/mock_location_provider.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -27,30 +28,7 @@ using testing::MatcherInterface;
 using testing::MatchResultListener;
 
 namespace device {
-
-class LocationProviderForTestArbitrator : public GeolocationProviderImpl {
- public:
-  LocationProviderForTestArbitrator() : mock_arbitrator_(NULL) {}
-  ~LocationProviderForTestArbitrator() override {}
-
-  // Only valid for use on the geolocation thread.
-  MockLocationArbitrator* mock_arbitrator() const { return mock_arbitrator_; }
-
- protected:
-  // GeolocationProviderImpl implementation:
-  std::unique_ptr<LocationArbitrator> CreateArbitrator() override;
-
- private:
-  // An alias to the arbitrator stored in the super class, where it is owned.
-  MockLocationArbitrator* mock_arbitrator_;
-};
-
-std::unique_ptr<LocationArbitrator>
-LocationProviderForTestArbitrator::CreateArbitrator() {
-  DCHECK(mock_arbitrator_ == NULL);
-  mock_arbitrator_ = new MockLocationArbitrator;
-  return base::WrapUnique(mock_arbitrator_);
-}
+namespace {
 
 class GeolocationObserver {
  public:
@@ -113,14 +91,24 @@ Matcher<const Geoposition&> GeopositionEq(const Geoposition& expected) {
   return MakeMatcher(new GeopositionEqMatcher(expected));
 }
 
+void DummyFunction(const LocationProvider* provider,
+                   const Geoposition& position) {}
+
+}  // namespace
+
 class GeolocationProviderTest : public testing::Test {
  protected:
-  GeolocationProviderTest()
-      : message_loop_(), provider_(new LocationProviderForTestArbitrator) {}
+  GeolocationProviderTest() : arbitrator_(new MockLocationProvider) {
+    provider()->SetArbitratorForTesting(base::WrapUnique(arbitrator_));
+  }
 
   ~GeolocationProviderTest() override {}
 
-  LocationProviderForTestArbitrator* provider() { return provider_.get(); }
+  GeolocationProviderImpl* provider() {
+    return GeolocationProviderImpl::GetInstance();
+  }
+
+  MockLocationProvider* arbitrator() { return arbitrator_; }
 
   // Called on test thread.
   bool ProvidersStarted();
@@ -130,15 +118,25 @@ class GeolocationProviderTest : public testing::Test {
   // Called on provider thread.
   void GetProvidersStarted(bool* started);
 
+  // |at_exit| must be initialized before all other variables so that it is
+  // available to register with Singletons and can handle tear down when the
+  // test completes.
+  base::ShadowingAtExitManager at_exit_;
+
   base::MessageLoopForUI message_loop_;
-  std::unique_ptr<LocationProviderForTestArbitrator> provider_;
+
+  // Owned by the GeolocationProviderImpl class.
+  MockLocationProvider* arbitrator_;
+
+  DISALLOW_COPY_AND_ASSIGN(GeolocationProviderTest);
 };
 
 bool GeolocationProviderTest::ProvidersStarted() {
-  DCHECK(provider_->IsRunning());
+  DCHECK(provider()->IsRunning());
   DCHECK(base::MessageLoop::current() == &message_loop_);
+
   bool started;
-  provider_->task_runner()->PostTaskAndReply(
+  provider()->task_runner()->PostTaskAndReply(
       FROM_HERE, base::Bind(&GeolocationProviderTest::GetProvidersStarted,
                             base::Unretained(this), &started),
       base::MessageLoop::QuitWhenIdleClosure());
@@ -147,33 +145,35 @@ bool GeolocationProviderTest::ProvidersStarted() {
 }
 
 void GeolocationProviderTest::GetProvidersStarted(bool* started) {
-  DCHECK(provider_->task_runner()->BelongsToCurrentThread());
-  *started = provider_->mock_arbitrator()->providers_started();
+  DCHECK(provider()->task_runner()->BelongsToCurrentThread());
+  *started = arbitrator()->is_started();
 }
 
 void GeolocationProviderTest::SendMockLocation(const Geoposition& position) {
-  DCHECK(provider_->IsRunning());
+  DCHECK(provider()->IsRunning());
   DCHECK(base::MessageLoop::current() == &message_loop_);
-  provider_->task_runner()->PostTask(
-      FROM_HERE, base::Bind(&GeolocationProviderImpl::OnLocationUpdate,
-                            base::Unretained(provider_.get()), position));
+  provider()->task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&GeolocationProviderImpl::OnLocationUpdate,
+                 base::Unretained(provider()), arbitrator_, position));
 }
 
 // Regression test for http://crbug.com/59377
 TEST_F(GeolocationProviderTest, OnPermissionGrantedWithoutObservers) {
+  // Clear |provider|'s arbitrator so the default arbitrator can be used.
+  provider()->SetArbitratorForTesting(nullptr);
   EXPECT_FALSE(provider()->user_did_opt_into_location_services_for_testing());
   provider()->UserDidOptIntoLocationServices();
   EXPECT_TRUE(provider()->user_did_opt_into_location_services_for_testing());
 }
 
-void DummyFunction(const Geoposition& position) {}
-
 TEST_F(GeolocationProviderTest, StartStop) {
   EXPECT_FALSE(provider()->IsRunning());
-  GeolocationProviderImpl::LocationUpdateCallback callback =
+  LocationProvider::LocationProviderUpdateCallback callback =
       base::Bind(&DummyFunction);
   std::unique_ptr<GeolocationProvider::Subscription> subscription =
-      provider()->AddLocationUpdateCallback(callback, false);
+      provider()->AddLocationUpdateCallback(base::Bind(callback, arbitrator()),
+                                            false);
   EXPECT_TRUE(provider()->IsRunning());
   EXPECT_TRUE(ProvidersStarted());
 

device/geolocation/location_arbitrator.h

-// Copyright (c) 2012 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 DEVICE_GEOLOCATION_LOCATION_ARBITRATOR_H_
-#define DEVICE_GEOLOCATION_LOCATION_ARBITRATOR_H_
-
-#include "device/geolocation/geolocation_export.h"
-
-namespace device {
-
-// This class is responsible for handling updates from multiple underlying
-// providers and resolving them to a single 'best' location fix at any given
-// moment.
-class DEVICE_GEOLOCATION_EXPORT LocationArbitrator {
- public:
-  virtual ~LocationArbitrator(){};
-
-  // See more details in geolocation_provider.
-  virtual void StartProviders(bool enable_high_accuracy) = 0;
-  virtual void StopProviders() = 0;
-
-  // Called everytime permission is granted to a page for using geolocation.
-  // This may either be through explicit user action (e.g. responding to the
-  // infobar prompt) or inferred from a persisted site permission.
-  // The arbitrator will inform all providers of this, which may in turn use
-  // this information to modify their internal policy.
-  virtual void OnPermissionGranted() = 0;
-
-  // Returns true if this arbitrator has received at least one call to
-  // OnPermissionGranted().
-  virtual bool HasPermissionBeenGranted() const = 0;
-};
-
-}  // namespace device
-
-#endif  // DEVICE_GEOLOCATION_LOCATION_ARBITRATOR_H_

device/geolocation/location_arbitrator_impl.cc

@@ -5,6 +5,8 @@
 #include "device/geolocation/location_arbitrator_impl.h"
 
 #include <map>
+#include <memory>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -28,13 +30,8 @@ const int64_t LocationArbitratorImpl::kFixStaleTimeoutMilliseconds =
     11 * base::Time::kMillisecondsPerSecond;
 
 LocationArbitratorImpl::LocationArbitratorImpl(
-    const LocationUpdateCallback& callback,
     GeolocationDelegate* delegate)
     : delegate_(delegate),
-      arbitrator_update_callback_(callback),
-      provider_update_callback_(
-          base::Bind(&LocationArbitratorImpl::OnLocationUpdate,
-                     base::Unretained(this))),
       position_provider_(nullptr),
       is_permission_granted_(false),
       is_running_(false) {}
@@ -45,13 +42,17 @@ GURL LocationArbitratorImpl::DefaultNetworkProviderURL() {
   return GURL(kDefaultNetworkProviderUrl);
 }
 
+bool LocationArbitratorImpl::HasPermissionBeenGrantedForTest() const {
+  return is_permission_granted_;
+}
+
 void LocationArbitratorImpl::OnPermissionGranted() {
   is_permission_granted_ = true;
   for (const auto& provider : providers_)
     provider->OnPermissionGranted();
 }
 
-void LocationArbitratorImpl::StartProviders(bool enable_high_accuracy) {
+bool LocationArbitratorImpl::StartProvider(bool enable_high_accuracy) {
   // Stash options as OnAccessTokenStoresLoaded has not yet been called.
   is_running_ = true;
   enable_high_accuracy_ = enable_high_accuracy;
@@ -67,26 +68,29 @@ void LocationArbitratorImpl::StartProviders(bool enable_high_accuracy) {
           base::Bind(&LocationArbitratorImpl::OnAccessTokenStoresLoaded,
                      base::Unretained(this)));
       access_token_store->LoadAccessTokens(token_store_callback_.callback());
-      return;
+      return true;
     }
   }
-  DoStartProviders();
+  return DoStartProviders();
 }
 
-void LocationArbitratorImpl::DoStartProviders() {
+bool LocationArbitratorImpl::DoStartProviders() {
   if (providers_.empty()) {
     // If no providers are available, we report an error to avoid
     // callers waiting indefinitely for a reply.
     Geoposition position;
     position.error_code = Geoposition::ERROR_CODE_PERMISSION_DENIED;
-    arbitrator_update_callback_.Run(position);
-    return;
+    arbitrator_update_callback_.Run(this, position);
+    return false;
   }
-  for (const auto& provider : providers_)
-    provider->StartProvider(enable_high_accuracy_);
+  bool started = false;
+  for (const auto& provider : providers_) {
+    started = provider->StartProvider(enable_high_accuracy_) || started;
+  }
+  return started;
 }
 
-void LocationArbitratorImpl::StopProviders() {
+void LocationArbitratorImpl::StopProvider() {
   // Reset the reference location state (provider+position)
   // so that future starts use fresh locations from
   // the newly constructed providers.
@@ -114,7 +118,8 @@ void LocationArbitratorImpl::RegisterProvider(
     std::unique_ptr<LocationProvider> provider) {
   if (!provider)
     return;
-  provider->SetUpdateCallback(provider_update_callback_);
+  provider->SetUpdateCallback(base::Bind(
+      &LocationArbitratorImpl::OnLocationUpdate, base::Unretained(this)));
   if (is_permission_granted_)
     provider->OnPermissionGranted();
   providers_.push_back(std::move(provider));
@@ -137,7 +142,17 @@ void LocationArbitratorImpl::OnLocationUpdate(const LocationProvider* provider,
     return;
   position_provider_ = provider;
   position_ = new_position;
-  arbitrator_update_callback_.Run(position_);
+  arbitrator_update_callback_.Run(this, position_);
+}
+
+const Geoposition& LocationArbitratorImpl::GetPosition() {
+  return position_;
+}
+
+void LocationArbitratorImpl::SetUpdateCallback(
+    const LocationProviderUpdateCallback& callback) {
+  DCHECK(!callback.is_null());
+  arbitrator_update_callback_ = callback;
 }
 
 scoped_refptr<AccessTokenStore> LocationArbitratorImpl::NewAccessTokenStore() {
@@ -205,8 +220,4 @@ bool LocationArbitratorImpl::IsNewPositionBetter(
   return false;
 }
 
-bool LocationArbitratorImpl::HasPermissionBeenGranted() const {
-  return is_permission_granted_;
-}
-
 }  // namespace device

device/geolocation/location_arbitrator_impl.h

@@ -6,6 +6,7 @@
 #define DEVICE_GEOLOCATION_LOCATION_ARBITRATOR_IMPL_H_
 
 #include <stdint.h>
+#include <memory>
 #include <vector>
 
 #include "base/callback_forward.h"
@@ -18,7 +19,6 @@
 #include "device/geolocation/geolocation_export.h"
 #include "device/geolocation/geolocation_provider.h"
 #include "device/geolocation/geoposition.h"
-#include "device/geolocation/location_arbitrator.h"
 #include "device/geolocation/location_provider.h"
 #include "net/url_request/url_request_context_getter.h"
 
@@ -35,26 +35,26 @@ class LocationProvider;
 // providers and resolving them to a single 'best' location fix at any given
 // moment.
 class DEVICE_GEOLOCATION_EXPORT LocationArbitratorImpl
-    : public LocationArbitrator {
+    : public LocationProvider {
  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_t kFixStaleTimeoutMilliseconds;
 
-  typedef base::Callback<void(const Geoposition&)> LocationUpdateCallback;
-
-  LocationArbitratorImpl(const LocationUpdateCallback& callback,
-                         GeolocationDelegate* delegate);
+  explicit LocationArbitratorImpl(GeolocationDelegate* delegate);
   ~LocationArbitratorImpl() override;
 
   static GURL DefaultNetworkProviderURL();
-
-  // LocationArbitrator
-  void StartProviders(bool enable_high_accuracy) override;
-  void StopProviders() override;
+  bool HasPermissionBeenGrantedForTest() const;
+
+  // LocationProvider implementation.
+  void SetUpdateCallback(
+      const LocationProviderUpdateCallback& callback) override;
+  bool StartProvider(bool enable_high_accuracy) override;
+  void StopProvider() override;
+  const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
-  bool HasPermissionBeenGranted() const override;
 
  protected:
   // These functions are useful for injection of dependencies in derived
@@ -81,7 +81,7 @@ class DEVICE_GEOLOCATION_EXPORT LocationArbitratorImpl
   void OnAccessTokenStoresLoaded(
       AccessTokenStore::AccessTokenMap access_token_map,
       const scoped_refptr<net::URLRequestContextGetter>& context_getter);
-  void DoStartProviders();
+  bool DoStartProviders();
 
   // Gets called when a provider has a new position.
   void OnLocationUpdate(const LocationProvider* provider,
@@ -97,8 +97,7 @@ class DEVICE_GEOLOCATION_EXPORT LocationArbitratorImpl
   GeolocationDelegate* delegate_;
 
   scoped_refptr<AccessTokenStore> access_token_store_;
-  LocationUpdateCallback arbitrator_update_callback_;
-  LocationProvider::LocationProviderUpdateCallback provider_update_callback_;
+  LocationProvider::LocationProviderUpdateCallback arbitrator_update_callback_;
 
   // The CancelableCallback will prevent OnAccessTokenStoresLoaded from being
   // called multiple times by calling Reset() at the time of binding.

device/geolocation/location_arbitrator_impl_unittest.cc

@@ -5,6 +5,7 @@
 #include "device/geolocation/location_arbitrator_impl.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
@@ -29,7 +30,7 @@ class MockLocationObserver {
     ASSERT_FALSE(last_position_.Validate());
   }
   // Delegate
-  void OnLocationUpdate(const Geoposition& position) {
+  void OnLocationUpdate(const LocationProvider*, const Geoposition& position) {
     last_position_ = position;
   }
 
@@ -95,13 +96,15 @@ class FakeGeolocationDelegate : public GeolocationDelegate {
 class TestingLocationArbitrator : public LocationArbitratorImpl {
  public:
   TestingLocationArbitrator(
-      const LocationArbitratorImpl::LocationUpdateCallback& callback,
+      const LocationProviderUpdateCallback& callback,
       const scoped_refptr<AccessTokenStore>& access_token_store,
       GeolocationDelegate* delegate)
-      : LocationArbitratorImpl(callback, delegate),
+      : LocationArbitratorImpl(delegate),
         cell_(nullptr),
         gps_(nullptr),
-        access_token_store_(access_token_store) {}
+        access_token_store_(access_token_store) {
+    SetUpdateCallback(callback);
+  }
 
   base::Time GetTimeNow() const override { return GetTimeNowForTest(); }
 
@@ -146,7 +149,7 @@ class GeolocationLocationArbitratorTest : public testing::Test {
   void InitializeArbitrator(std::unique_ptr<GeolocationDelegate> delegate) {
     if (delegate)
       delegate_ = std::move(delegate);
-    const LocationArbitratorImpl::LocationUpdateCallback callback =
+    const LocationProvider::LocationProviderUpdateCallback callback =
         base::Bind(&MockLocationObserver::OnLocationUpdate,
                    base::Unretained(observer_.get()));
     arbitrator_.reset(new TestingLocationArbitrator(
@@ -193,9 +196,9 @@ TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
 
 TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
   InitializeArbitrator(nullptr);
-  EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_FALSE(arbitrator_->HasPermissionBeenGrantedForTest());
   arbitrator_->OnPermissionGranted();
-  EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_TRUE(arbitrator_->HasPermissionBeenGrantedForTest());
   // Can't check the provider has been notified without going through the
   // motions to create the provider (see next test).
   EXPECT_FALSE(cell());
@@ -209,7 +212,7 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
 
   EXPECT_TRUE(access_token_store_->access_token_map_.empty());
 
@@ -226,13 +229,13 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   EXPECT_TRUE(observer_->last_position_.Validate() ||
               observer_->last_position_.error_code !=
                   Geoposition::ERROR_CODE_NONE);
-  EXPECT_EQ(cell()->position_.latitude, observer_->last_position_.latitude);
+  EXPECT_EQ(cell()->position().latitude, observer_->last_position_.latitude);
 
-  EXPECT_FALSE(cell()->is_permission_granted_);
-  EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_FALSE(cell()->is_permission_granted());
+  EXPECT_FALSE(arbitrator_->HasPermissionBeenGrantedForTest());
   arbitrator_->OnPermissionGranted();
-  EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
-  EXPECT_TRUE(cell()->is_permission_granted_);
+  EXPECT_TRUE(arbitrator_->HasPermissionBeenGrantedForTest());
+  EXPECT_TRUE(cell()->is_permission_granted());
 }
 
 TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
@@ -245,7 +248,7 @@ TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
 
   ASSERT_FALSE(cell());
   EXPECT_FALSE(gps());
@@ -260,14 +263,15 @@ TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
   EXPECT_TRUE(observer_->last_position_.Validate() ||
               observer_->last_position_.error_code !=
                   Geoposition::ERROR_CODE_NONE);
-  EXPECT_EQ(fake_delegate->mock_location_provider()->position_.latitude,
+  EXPECT_EQ(fake_delegate->mock_location_provider()->position().latitude,
             observer_->last_position_.latitude);
 
-  EXPECT_FALSE(fake_delegate->mock_location_provider()->is_permission_granted_);
-  EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_FALSE(
+      fake_delegate->mock_location_provider()->is_permission_granted());
+  EXPECT_FALSE(arbitrator_->HasPermissionBeenGrantedForTest());
   arbitrator_->OnPermissionGranted();
-  EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
-  EXPECT_TRUE(fake_delegate->mock_location_provider()->is_permission_granted_);
+  EXPECT_TRUE(arbitrator_->HasPermissionBeenGrantedForTest());
+  EXPECT_TRUE(fake_delegate->mock_location_provider()->is_permission_granted());
 }
 
 TEST_F(GeolocationLocationArbitratorTest,
@@ -281,7 +285,7 @@ TEST_F(GeolocationLocationArbitratorTest,
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
 
   EXPECT_TRUE(access_token_store_->access_token_map_.empty());
 
@@ -301,18 +305,18 @@ TEST_F(GeolocationLocationArbitratorTest,
   EXPECT_TRUE(observer_->last_position_.Validate() ||
               observer_->last_position_.error_code !=
                   Geoposition::ERROR_CODE_NONE);
-  EXPECT_EQ(cell()->position_.latitude, observer_->last_position_.latitude);
+  EXPECT_EQ(cell()->position().latitude, observer_->last_position_.latitude);
 
-  EXPECT_FALSE(cell()->is_permission_granted_);
-  EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_FALSE(cell()->is_permission_granted());
+  EXPECT_FALSE(arbitrator_->HasPermissionBeenGrantedForTest());
   arbitrator_->OnPermissionGranted();
-  EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
-  EXPECT_TRUE(cell()->is_permission_granted_);
+  EXPECT_TRUE(arbitrator_->HasPermissionBeenGrantedForTest());
+  EXPECT_TRUE(cell()->is_permission_granted());
 }
 
 TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
   InitializeArbitrator(nullptr);
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
@@ -321,14 +325,14 @@ TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
   SetReferencePosition(cell());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
-  arbitrator_->StartProviders(true);
+  arbitrator_->StartProvider(true);
   EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, gps()->state_);
 }
 
 TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
   InitializeArbitrator(nullptr);
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
@@ -351,7 +355,7 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
 
   // Advance time, and notify once again
   AdvanceTimeNow(SwitchOnFreshnessCliff());
-  cell()->HandlePositionChanged(cell()->position_);
+  cell()->HandlePositionChanged(cell()->position());
 
   // New fix is available, less accurate but fresher
   CheckLastPositionInfo(5, 6, 150);
@@ -405,7 +409,7 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
 
 TEST_F(GeolocationLocationArbitratorTest, TwoOneShotsIsNewPositionBetter) {
   InitializeArbitrator(nullptr);
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
@@ -415,14 +419,14 @@ TEST_F(GeolocationLocationArbitratorTest, TwoOneShotsIsNewPositionBetter) {
   CheckLastPositionInfo(3, 139, 100);
 
   // Restart providers to simulate a one-shot request.
-  arbitrator_->StopProviders();
+  arbitrator_->StopProvider();
 
   // To test 240956, perform a throwaway alloc.
   // This convinces the allocator to put the providers in a new memory location.
   std::unique_ptr<MockLocationProvider> dummy_provider(
       new MockLocationProvider);
 
-  arbitrator_->StartProviders(false);
+  arbitrator_->StartProvider(false);
   access_token_store_->NotifyDelegateTokensLoaded();
 
   // Advance the time a short while to simulate successive calls.

device/geolocation/location_provider.h

@@ -14,7 +14,7 @@ namespace device {
 class LocationProvider;
 
 // The interface for providing location information.
-class LocationProvider {
+class DEVICE_GEOLOCATION_EXPORT LocationProvider {
  public:
   virtual ~LocationProvider() {}
 
@@ -40,11 +40,7 @@ class LocationProvider {
   virtual void StopProvider() = 0;
 
   // Gets the current best position estimate.
-  virtual void GetPosition(Geoposition* position) = 0;
-
-  // Provides a hint to the provider that new location data is needed as soon
-  // as possible.
-  virtual void RequestRefresh() = 0;
+  virtual const Geoposition& GetPosition() = 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

device/geolocation/location_provider_android.cc

@@ -4,6 +4,8 @@
 
 #include "device/geolocation/location_provider_android.h"
 
+#include <memory>
+
 #include "base/memory/ptr_util.h"
 #include "base/time/time.h"
 #include "device/geolocation/geoposition.h"
@@ -32,12 +34,8 @@ void LocationProviderAndroid::StopProvider() {
   AndroidLocationApiAdapter::GetInstance()->Stop();
 }
 
-void LocationProviderAndroid::GetPosition(Geoposition* position) {
-  *position = last_position_;
-}
-
-void LocationProviderAndroid::RequestRefresh() {
-  // Nothing to do here, android framework will call us back on new position.
+const Geoposition& LocationProviderAndroid::GetPosition() {
+  return last_position_;
 }
 
 void LocationProviderAndroid::OnPermissionGranted() {

device/geolocation/location_provider_android.h

@@ -25,8 +25,7 @@ class LocationProviderAndroid : public LocationProviderBase {
   // LocationProvider.
   bool StartProvider(bool high_accuracy) override;
   void StopProvider() override;
-  void GetPosition(Geoposition* position) override;
-  void RequestRefresh() override;
+  const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
 
  private:

device/geolocation/location_provider_base.cc

@@ -20,6 +20,4 @@ void LocationProviderBase::SetUpdateCallback(
   callback_ = callback;
 }
 
-void LocationProviderBase::RequestRefresh() {}
-
 }  // namespace device

device/geolocation/location_provider_base.h

@@ -23,7 +23,6 @@ class DEVICE_GEOLOCATION_EXPORT LocationProviderBase
   // Overridden from LocationProvider:
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;
-  void RequestRefresh() override;
 
  private:
   LocationProviderUpdateCallback callback_;

device/geolocation/mock_location_arbitrator.cc

-// Copyright (c) 2012 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 "device/geolocation/mock_location_arbitrator.h"
-
-#include "base/message_loop/message_loop.h"
-#include "device/geolocation/geoposition.h"
-
-namespace device {
-
-MockLocationArbitrator::MockLocationArbitrator()
-    : permission_granted_(false), providers_started_(false) {}
-
-void MockLocationArbitrator::StartProviders(bool enable_high_accuracy) {
-  providers_started_ = true;
-}
-
-void MockLocationArbitrator::StopProviders() {
-  providers_started_ = false;
-}
-
-void MockLocationArbitrator::OnPermissionGranted() {
-  permission_granted_ = true;
-}
-
-bool MockLocationArbitrator::HasPermissionBeenGranted() const {
-  return permission_granted_;
-}
-
-}  // namespace device

device/geolocation/mock_location_arbitrator.h

-// Copyright (c) 2012 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 DEVICE_GEOLOCATION_MOCK_LOCATION_ARBITRATOR_H_
-#define DEVICE_GEOLOCATION_MOCK_LOCATION_ARBITRATOR_H_
-
-#include "base/compiler_specific.h"
-#include "base/macros.h"
-#include "device/geolocation/location_arbitrator.h"
-
-namespace device {
-
-struct Geoposition;
-
-class MockLocationArbitrator : public LocationArbitrator {
- public:
-  MockLocationArbitrator();
-
-  bool providers_started() const { return providers_started_; }
-
-  // LocationArbitrator:
-  void StartProviders(bool enable_high_accuracy) override;
-  void StopProviders() override;
-  void OnPermissionGranted() override;
-  bool HasPermissionBeenGranted() const override;
-
- private:
-  bool permission_granted_;
-  bool providers_started_;
-
-  DISALLOW_COPY_AND_ASSIGN(MockLocationArbitrator);
-};
-
-}  // namespace device
-
-#endif  // DEVICE_GEOLOCATION_MOCK_LOCATION_ARBITRATOR_H_

device/geolocation/mock_location_provider.cc

@@ -46,79 +46,12 @@ void MockLocationProvider::StopProvider() {
   state_ = STOPPED;
 }
 
-void MockLocationProvider::GetPosition(Geoposition* position) {
-  *position = position_;
+const Geoposition& MockLocationProvider::GetPosition() {
+  return position_;
 }
 
 void MockLocationProvider::OnPermissionGranted() {
   is_permission_granted_ = true;
 }
 
-// Mock location provider that automatically calls back its client at most
-// once, when StartProvider or OnPermissionGranted is called. Use
-// |requires_permission_to_start| to select which event triggers the callback.
-class AutoMockLocationProvider : public MockLocationProvider {
- public:
-  AutoMockLocationProvider(bool has_valid_location,
-                           bool requires_permission_to_start)
-      : requires_permission_to_start_(requires_permission_to_start),
-        listeners_updated_(false) {
-    if (has_valid_location) {
-      position_.accuracy = 3;
-      position_.latitude = 4.3;
-      position_.longitude = -7.8;
-      // Webkit compares the timestamp to wall clock time, so we need it to be
-      // contemporary.
-      position_.timestamp = base::Time::Now();
-    } else {
-      position_.error_code = Geoposition::ERROR_CODE_POSITION_UNAVAILABLE;
-    }
-  }
-  bool StartProvider(bool high_accuracy) override {
-    MockLocationProvider::StartProvider(high_accuracy);
-    if (!requires_permission_to_start_) {
-      UpdateListenersIfNeeded();
-    }
-    return true;
-  }
-
-  void OnPermissionGranted() override {
-    MockLocationProvider::OnPermissionGranted();
-    if (requires_permission_to_start_) {
-      UpdateListenersIfNeeded();
-    }
-  }
-
-  void UpdateListenersIfNeeded() {
-    if (!listeners_updated_) {
-      listeners_updated_ = true;
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&MockLocationProvider::HandlePositionChanged,
-                                base::Unretained(this), position_));
-    }
-  }
-
- private:
-  const bool requires_permission_to_start_;
-  bool listeners_updated_;
-
-  DISALLOW_COPY_AND_ASSIGN(AutoMockLocationProvider);
-};
-
-LocationProvider* NewMockLocationProvider() {
-  return new MockLocationProvider;
-}
-
-LocationProvider* NewAutoSuccessMockLocationProvider() {
-  return new AutoMockLocationProvider(true, false);
-}
-
-LocationProvider* NewAutoFailMockLocationProvider() {
-  return new AutoMockLocationProvider(false, false);
-}
-
-LocationProvider* NewAutoSuccessMockNetworkLocationProvider() {
-  return new AutoMockLocationProvider(true, true);
-}
-
 }  // namespace device

device/geolocation/mock_location_provider.h

@@ -23,41 +23,27 @@ class MockLocationProvider : public LocationProviderBase {
   MockLocationProvider();
   ~MockLocationProvider() override;
 
+  bool is_started() const { return state_ != STOPPED; }
+  bool is_permission_granted() const { return is_permission_granted_; }
+  const Geoposition& position() const { return position_; }
+
   // Updates listeners with the new position.
   void HandlePositionChanged(const Geoposition& position);
 
   // LocationProvider implementation.
   bool StartProvider(bool high_accuracy) override;
   void StopProvider() override;
-  void GetPosition(Geoposition* position) override;
+  const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
 
+ private:
   bool is_permission_granted_;
   Geoposition position_;
   scoped_refptr<base::SingleThreadTaskRunner> provider_task_runner_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(MockLocationProvider);
 };
 
-// Factory functions for the various sorts of mock location providers,
-// for use with LocationArbitrator::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 factory method).
-
-// Creates a mock location provider with no default behavior.
-LocationProvider* NewMockLocationProvider();
-// Creates a mock location provider that automatically notifies its
-// listeners with a valid location when StartProvider is called.
-LocationProvider* NewAutoSuccessMockLocationProvider();
-// Creates a mock location provider that automatically notifies its
-// listeners with an error when StartProvider is called.
-LocationProvider* NewAutoFailMockLocationProvider();
-// Similar to NewAutoSuccessMockLocationProvider but mimicks the behavior of
-// the Network Location provider, in deferring making location updates until
-// a permission request has been confirmed.
-LocationProvider* NewAutoSuccessMockNetworkLocationProvider();
-
 }  // namespace device
 
 #endif  // DEVICE_GEOLOCATION_MOCK_LOCATION_PROVIDER_H_

device/geolocation/network_location_provider.cc

@@ -4,6 +4,8 @@
 
 #include "device/geolocation/network_location_provider.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ -128,26 +130,15 @@ NetworkLocationProvider::~NetworkLocationProvider() {
 }
 
 // LocationProvider implementation
-void NetworkLocationProvider::GetPosition(Geoposition* position) {
-  DCHECK(position);
-  *position = position_;
-}
-
-void NetworkLocationProvider::RequestRefresh() {
-  // TODO(joth): When called via the public (base class) interface, this should
-  // poke each data provider to get them to expedite their next scan.
-  // Whilst in the delayed start, only send request if all data is ready.
-  // TODO(mcasas): consider not using HasWeakPtrs() https://crbug.com/629158.
-  if (!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) {
-    RequestPosition();
-  }
+const Geoposition& NetworkLocationProvider::GetPosition() {
+  return position_;
 }
 
 void NetworkLocationProvider::OnPermissionGranted() {
   const bool was_permission_granted = is_permission_granted_;
   is_permission_granted_ = true;
   if (!was_permission_granted && IsStarted()) {
-    RequestRefresh();
+    RequestPosition();
   }
 }
 
@@ -212,7 +203,7 @@ void NetworkLocationProvider::OnWifiDataUpdated() {
   wifi_timestamp_ = base::Time::Now();
 
   is_new_data_available_ = is_wifi_data_complete_;
-  RequestRefresh();
+  RequestPosition();
 }
 
 void NetworkLocationProvider::StopProvider() {
@@ -227,6 +218,10 @@ void NetworkLocationProvider::StopProvider() {
 // Other methods
 void NetworkLocationProvider::RequestPosition() {
   DCHECK(CalledOnValidThread());
+
+  // TODO(mcasas): consider not using HasWeakPtrs() https://crbug.com/629158.
+  if (weak_factory_.HasWeakPtrs() && !is_wifi_data_complete_)
+    return;
   if (!is_new_data_available_)
     return;
 
@@ -238,11 +233,13 @@ void NetworkLocationProvider::RequestPosition() {
     DCHECK(cached_position->Validate());
     // Record the position and update its timestamp.
     position_ = *cached_position;
+
     // The timestamp of a position fix is determined by the timestamp
     // of the source data update. (The value of position_.timestamp from
     // the cache could be from weeks ago!)
     position_.timestamp = wifi_timestamp_;
     is_new_data_available_ = false;
+
     // Let listeners know that we now have a position available.
     NotifyCallback(position_);
     return;

device/geolocation/network_location_provider.h

@@ -73,8 +73,7 @@ class NetworkLocationProvider : public base::NonThreadSafe,
   // LocationProvider implementation
   bool StartProvider(bool high_accuracy) override;
   void StopProvider() override;
-  void GetPosition(Geoposition* position) override;
-  void RequestRefresh() override;
+  const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
 
  private:

device/geolocation/network_location_provider_unittest.cc

@@ -7,7 +7,9 @@
 #include <stddef.h>
 
 #include <memory>
+#include <string>
 #include <utility>
+#include <vector>
 
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
@@ -277,9 +279,9 @@ class GeolocationNetworkProviderTest : public testing::Test {
     ASSERT_TRUE(parsed_json->GetAsDictionary(&request_json));
 
     if (!is_default_url) {
-      if (expected_access_token.empty())
+      if (expected_access_token.empty()) {
         ASSERT_FALSE(request_json->HasKey(kAccessTokenString));
-      else {
+      } else {
         std::string access_token;
         EXPECT_TRUE(request_json->GetString(kAccessTokenString, &access_token));
         EXPECT_EQ(expected_access_token, access_token);
@@ -381,8 +383,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   fetcher->SetResponseString(kNoFixNetworkResponse);
   fetcher->delegate()->OnURLFetchComplete(fetcher);
 
-  Geoposition position;
-  provider->GetPosition(&position);
+  Geoposition position = provider->GetPosition();
   EXPECT_FALSE(position.Validate());
 
   // Now wifi data arrives -- SetData will notify listeners.
@@ -411,7 +412,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   fetcher->SetResponseString(kReferenceNetworkResponse);
   fetcher->delegate()->OnURLFetchComplete(fetcher);
 
-  provider->GetPosition(&position);
+  position = provider->GetPosition();
   EXPECT_EQ(51.0, position.latitude);
   EXPECT_EQ(-0.1, position.longitude);
   EXPECT_EQ(1200.4, position.accuracy);
@@ -430,7 +431,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   fetcher = get_url_fetcher_and_advance_id();
   EXPECT_FALSE(fetcher);
 
-  provider->GetPosition(&position);
+  position = provider->GetPosition();
   EXPECT_EQ(51.0, position.latitude);
   EXPECT_EQ(-0.1, position.longitude);
   EXPECT_TRUE(position.Validate());
@@ -451,7 +452,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   fetcher->delegate()->OnURLFetchComplete(fetcher);
 
   // Error means we now no longer have a fix.
-  provider->GetPosition(&position);
+  position = provider->GetPosition();
   EXPECT_FALSE(position.Validate());
 
   // Wifi scan returns to original set: should be serviced from cache.
@@ -459,7 +460,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(get_url_fetcher_and_advance_id());  // No new request created.
 
-  provider->GetPosition(&position);
+  position = provider->GetPosition();
   EXPECT_EQ(51.0, position.latitude);
   EXPECT_EQ(-0.1, position.longitude);
   EXPECT_TRUE(position.Validate());