Return void from LocationProvider::StartProvider

This CL changes the return type of LocationProvider::StartProvider (and
related functions) from bool to void.

Currently, no-one inspects the value returned from this function, except
to pass it through to other places that don't inspect it.

Most impls always return true. The situation where an impl doesn't
always return true (LocationArbitrator) are responded-to by other means.
Specifically, LocationArbitrator provides an Error geoposition in the
case that it couldn't create any location providers.

I also opportunistically removed the unused MockLocationProvider in this
CL.

Bug: 748921
Change-Id: Iaa7903b3eecaacc0076b3c1f1cce3be86187285d
Reviewed-on: https://chromium-review.googlesource.com/722139Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509561}

device/geolocation/BUILD.gn

@@ -159,8 +159,6 @@ source_set("test_support") {
     "fake_access_token_store.h",
     "fake_location_provider.cc",
     "fake_location_provider.h",
-    "mock_location_provider.cc",
-    "mock_location_provider.h",
   ]
   public_deps = [
     ":geolocation",

device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAdapter.java

@@ -44,7 +44,7 @@ public class LocationProviderAdapter {
      * @param enableHighAccuracy Whether or not to enable high accuracy location providers.
      */
     @CalledByNative
-    public boolean start(final boolean enableHighAccuracy) {
+    public void start(final boolean enableHighAccuracy) {
         FutureTask<Void> task = new FutureTask<Void>(new Runnable() {
             @Override
             public void run() {
@@ -52,7 +52,6 @@ public class LocationProviderAdapter {
             }
         }, null);
         ThreadUtils.runOnUiThread(task);
-        return true;
     }
 
     /**

device/geolocation/fake_location_provider.cc

@@ -43,9 +43,8 @@ void FakeLocationProvider::SetUpdateCallback(
   callback_ = callback;
 }
 
-bool FakeLocationProvider::StartProvider(bool high_accuracy) {
+void FakeLocationProvider::StartProvider(bool high_accuracy) {
   state_ = high_accuracy ? HIGH_ACCURACY : LOW_ACCURACY;
-  return true;
 }
 
 void FakeLocationProvider::StopProvider() {

device/geolocation/fake_location_provider.h

@@ -32,7 +32,7 @@ class FakeLocationProvider : public LocationProvider {
   // LocationProvider implementation.
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;
-  bool StartProvider(bool high_accuracy) override;
+  void StartProvider(bool high_accuracy) override;
   void StopProvider() override;
   const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;

device/geolocation/location_api_adapter_android.cc

@@ -42,7 +42,7 @@ static void NewErrorAvailable(JNIEnv* env,
 
 namespace device {
 
-bool LocationApiAdapterAndroid::Start(OnGeopositionCB on_geoposition_callback,
+void LocationApiAdapterAndroid::Start(OnGeopositionCB on_geoposition_callback,
                                       bool high_accuracy) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(on_geoposition_callback);
@@ -62,8 +62,8 @@ bool LocationApiAdapterAndroid::Start(OnGeopositionCB on_geoposition_callback,
   // start receiving notifications from java in the main thread looper until
   // Stop() is called.
   DCHECK(!java_location_provider_adapter_.is_null());
-  return Java_LocationProviderAdapter_start(
-      env, java_location_provider_adapter_, high_accuracy);
+  Java_LocationProviderAdapter_start(env, java_location_provider_adapter_,
+                                     high_accuracy);
 }
 
 void LocationApiAdapterAndroid::Stop() {

device/geolocation/location_api_adapter_android.h

@@ -34,9 +34,9 @@ class LocationApiAdapterAndroid {
  public:
   using OnGeopositionCB = base::Callback<void(const Geoposition&)>;
 
-  // Starts the underlying location provider, returns true if successful.
+  // Starts the underlying location provider.
   // Called on |task_runner_|.
-  bool Start(OnGeopositionCB on_geoposition_callback, bool high_accuracy);
+  void Start(OnGeopositionCB on_geoposition_callback, bool high_accuracy);
 
   // Stops the underlying location provider. Called on |task_runner_|.
   void Stop();

device/geolocation/location_arbitrator.cc

@@ -46,7 +46,7 @@ void LocationArbitrator::OnPermissionGranted() {
     provider->OnPermissionGranted();
 }
 
-bool LocationArbitrator::StartProvider(bool enable_high_accuracy) {
+void LocationArbitrator::StartProvider(bool enable_high_accuracy) {
   is_running_ = true;
   enable_high_accuracy_ = enable_high_accuracy;
 
@@ -66,26 +66,24 @@ bool LocationArbitrator::StartProvider(bool enable_high_accuracy) {
       // Invoke callback to obtain a URL request context.
       request_context_producer_.Run(
           request_context_response_callback_.callback());
-      return true;
+      return;
     }
   }
-  return DoStartProviders();
+  DoStartProviders();
 }
 
-bool LocationArbitrator::DoStartProviders() {
+void LocationArbitrator::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_POSITION_UNAVAILABLE;
     arbitrator_update_callback_.Run(this, position);
-    return false;
+    return;
   }
-  bool started = false;
   for (const auto& provider : providers_) {
-    started = provider->StartProvider(enable_high_accuracy_) || started;
+    provider->StartProvider(enable_high_accuracy_);
   }
-  return started;
 }
 
 void LocationArbitrator::StopProvider() {

device/geolocation/location_arbitrator.h

@@ -52,7 +52,7 @@ class DEVICE_GEOLOCATION_EXPORT LocationArbitrator : public LocationProvider {
   // LocationProvider implementation.
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;
-  bool StartProvider(bool enable_high_accuracy) override;
+  void StartProvider(bool enable_high_accuracy) override;
   void StopProvider() override;
   const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
@@ -77,8 +77,11 @@ class DEVICE_GEOLOCATION_EXPORT LocationArbitrator : public LocationProvider {
   void RegisterProvider(std::unique_ptr<LocationProvider> provider);
   void RegisterSystemProvider();
 
-  // Tell all registered providers to start.
-  bool DoStartProviders();
+  // Tells all registered providers to start.
+  // If |providers_| is empty, immediately provides
+  // Geoposition::ERROR_CODE_POSITION_UNAVAILABLE to the client via
+  // |arbitrator_update_callback_|.
+  void DoStartProviders();
 
   // Response callback for request_context_callback_.
   void OnRequestContextResponse(

device/geolocation/location_provider.h

@@ -25,9 +25,8 @@ class DEVICE_GEOLOCATION_EXPORT LocationProvider {
       const LocationProviderUpdateCallback& callback) = 0;
 
   // StartProvider maybe called multiple times, e.g. to alter the
-  // |high_accuracy| setting. Returns false if a fatal error was encountered
-  // which prevented the provider from starting.
-  virtual bool StartProvider(bool high_accuracy) = 0;
+  // |high_accuracy| setting.
+  virtual void StartProvider(bool high_accuracy) = 0;
 
   // Stops the provider from sending more requests.
   // Important: a LocationProvider may be instantiated and StartProvider() may

device/geolocation/location_provider_android.cc

@@ -35,9 +35,9 @@ void LocationProviderAndroid::SetUpdateCallback(
   callback_ = callback;
 }
 
-bool LocationProviderAndroid::StartProvider(bool high_accuracy) {
+void LocationProviderAndroid::StartProvider(bool high_accuracy) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return LocationApiAdapterAndroid::GetInstance()->Start(
+  LocationApiAdapterAndroid::GetInstance()->Start(
       base::Bind(&LocationProviderAndroid::NotifyNewGeoposition,
                  weak_ptr_factory_.GetWeakPtr()),
       high_accuracy);

device/geolocation/location_provider_android.h

@@ -25,7 +25,7 @@ class LocationProviderAndroid : public LocationProvider {
   // LocationProvider implementation.
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;
-  bool StartProvider(bool high_accuracy) override;
+  void StartProvider(bool high_accuracy) override;
   void StopProvider() override;
   const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;

device/geolocation/mock_location_provider.cc

-// Copyright 2016 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_provider.h"
-
-namespace device {
-
-MockLocationProvider::MockLocationProvider() {}
-
-MockLocationProvider::~MockLocationProvider() {}
-
-}  // namespace device

device/geolocation/mock_location_provider.h

-// Copyright 2016 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_PROVIDER_H_
-#define DEVICE_GEOLOCATION_MOCK_LOCATION_PROVIDER_H_
-
-#include "device/geolocation/geoposition.h"
-#include "device/geolocation/location_provider.h"
-#include "testing/gmock/include/gmock/gmock.h"
-
-namespace device {
-
-class MockLocationProvider : public device::LocationProvider {
- public:
-  MockLocationProvider();
-  ~MockLocationProvider() override;
-
-  MOCK_METHOD1(SetUpdateCallback,
-               void(const LocationProviderUpdateCallback& callback));
-  MOCK_METHOD1(StartProvider, bool(bool high_accuracy));
-  MOCK_METHOD0(StopProvider, void());
-  MOCK_METHOD0(GetPosition, const Geoposition&());
-  MOCK_METHOD0(OnPermissionGranted, void());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockLocationProvider);
-};
-
-}  // namespace device
-
-#endif  // DEVICE_GEOLOCATION_MOCK_LOCATION_PROVIDER_H_

device/geolocation/network_location_provider.cc

@@ -157,11 +157,11 @@ void NetworkLocationProvider::OnLocationResponse(
     location_provider_update_callback_.Run(this, position_);
 }
 
-bool NetworkLocationProvider::StartProvider(bool high_accuracy) {
+void NetworkLocationProvider::StartProvider(bool high_accuracy) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (IsStarted())
-    return true;
+    return;
 
   // Registers a callback with the data provider. The first call to Register()
   // will create a singleton data provider that will be deleted on Unregister().
@@ -174,7 +174,6 @@ bool NetworkLocationProvider::StartProvider(bool high_accuracy) {
       base::TimeDelta::FromSeconds(kDataCompleteWaitSeconds));
 
   OnWifiDataUpdate();
-  return true;
 }
 
 void NetworkLocationProvider::StopProvider() {

device/geolocation/network_location_provider.h

@@ -68,7 +68,7 @@ class NetworkLocationProvider : public LocationProvider {
 
   // LocationProvider implementation
   void SetUpdateCallback(const LocationProviderUpdateCallback& cb) override;
-  bool StartProvider(bool high_accuracy) override;
+  void StartProvider(bool high_accuracy) override;
   void StopProvider() override;
   const Geoposition& GetPosition() override;
   void OnPermissionGranted() override;

device/geolocation/network_location_provider_unittest.cc

@@ -290,7 +290,7 @@ TEST_F(GeolocationNetworkProviderTest, CreateDestroy) {
 TEST_F(GeolocationNetworkProviderTest, EmptyApiKey) {
   const std::string api_key = "";
   std::unique_ptr<LocationProvider> provider(CreateProvider(true, api_key));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   ASSERT_TRUE(fetcher);
   EXPECT_FALSE(fetcher->GetOriginalURL().has_query());
@@ -301,7 +301,7 @@ TEST_F(GeolocationNetworkProviderTest, EmptyApiKey) {
 TEST_F(GeolocationNetworkProviderTest, NonEmptyApiKey) {
   const std::string api_key = "something";
   std::unique_ptr<LocationProvider> provider(CreateProvider(true, api_key));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   ASSERT_TRUE(fetcher);
   EXPECT_TRUE(fetcher->GetOriginalURL().has_query());
@@ -312,7 +312,7 @@ TEST_F(GeolocationNetworkProviderTest, NonEmptyApiKey) {
 // representing a valid request.
 TEST_F(GeolocationNetworkProviderTest, StartProvider) {
   std::unique_ptr<LocationProvider> provider(CreateProvider(true));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   ASSERT_TRUE(fetcher);
   CheckRequestIsValid(*fetcher, 0, 0);
@@ -322,7 +322,7 @@ TEST_F(GeolocationNetworkProviderTest, StartProvider) {
 // points represented in the request is truncated to fit within 2048 characters.
 TEST_F(GeolocationNetworkProviderTest, StartProviderLongRequest) {
   std::unique_ptr<LocationProvider> provider(CreateProvider(true));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   // Create Wifi scan data with too many access points.
   const int kFirstScanAps = 20;
   wifi_data_provider_->SetData(CreateReferenceWifiScanData(kFirstScanAps));
@@ -349,7 +349,7 @@ TEST_F(GeolocationNetworkProviderTest, StartProviderLongRequest) {
 // 7. Wifi data changes back to (2.) -> no new request, provide cached position.
 TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   std::unique_ptr<LocationProvider> provider(CreateProvider(true));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
 
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   ASSERT_TRUE(fetcher);
@@ -445,7 +445,7 @@ TEST_F(GeolocationNetworkProviderTest, NoRequestOnStartupUntilWifiData) {
   MessageLoopQuitListener listener;
   wifi_data_provider_->set_got_data(false);  // No initial Wifi data.
   std::unique_ptr<LocationProvider> provider(CreateProvider(true));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
 
   provider->SetUpdateCallback(base::Bind(
       &MessageLoopQuitListener::OnLocationUpdate, base::Unretained(&listener)));
@@ -466,7 +466,7 @@ TEST_F(GeolocationNetworkProviderTest, NoRequestOnStartupUntilWifiData) {
 TEST_F(GeolocationNetworkProviderTest, NewDataReplacesExistingNetworkRequest) {
   // Send initial request with empty data
   std::unique_ptr<LocationProvider> provider(CreateProvider(true));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   EXPECT_TRUE(fetcher);
 
@@ -482,7 +482,7 @@ TEST_F(GeolocationNetworkProviderTest, NewDataReplacesExistingNetworkRequest) {
 // user granting permission.
 TEST_F(GeolocationNetworkProviderTest, NetworkRequestDeferredForPermission) {
   std::unique_ptr<LocationProvider> provider(CreateProvider(false));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   EXPECT_FALSE(fetcher);
   provider->OnPermissionGranted();
@@ -496,7 +496,7 @@ TEST_F(GeolocationNetworkProviderTest, NetworkRequestDeferredForPermission) {
 TEST_F(GeolocationNetworkProviderTest,
        NetworkRequestWithWifiDataDeferredForPermission) {
   std::unique_ptr<LocationProvider> provider(CreateProvider(false));
-  EXPECT_TRUE(provider->StartProvider(false));
+  provider->StartProvider(false);
   net::TestURLFetcher* fetcher = get_url_fetcher_and_advance_id();
   EXPECT_FALSE(fetcher);