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

device/geolocation/location_arbitrator_impl.cc

Index: device/geolocation/location_arbitrator_impl.cc
diff --git a/device/geolocation/location_arbitrator_impl.cc b/device/geolocation/location_arbitrator_impl.cc
index a61a4a3ab22a867fba5d272f7d9961b12776bf23..f9040382358dfec1f1a385b42b93ba53b6034ca9 100644
--- a/device/geolocation/location_arbitrator_impl.cc
+++ b/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"
@@ -51,7 +53,7 @@ void LocationArbitratorImpl::OnPermissionGranted() {
     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 +69,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;
+    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;

[Kevin M, 2016/08/17 17:44:37]

super nit: put "started || before provider->StartProvider" to make the logical
OR absolutely clear to readers, as a hedge against coders' tendency to skim and
gloss over details. ;)

[CJ, 2016/08/17 23:07:11]

Because of short-circuiting, once the first StartProvider returns true and flips
started to true, we no longer will call StartProvider, thus to ensure
StartProvider gets called, it must be called first.  

[Wez, 2016/08/19 01:33:11]

Acknowledged.

+  }
+  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.
@@ -140,6 +145,16 @@ void LocationArbitratorImpl::OnLocationUpdate(const LocationProvider* provider,
   arbitrator_update_callback_.Run(position_);
 }
 
+const Geoposition& LocationArbitratorImpl::GetPosition() {
+  return position_;
+}
+
+void LocationArbitratorImpl::SetUpdateCallback(
+    const LocationProviderUpdateCallback& callback) {
+  DCHECK(!callback.is_null());
+  provider_update_callback_ = callback;

[Wez, 2016/08/19 01:56:30]

This doesn't look right - you should be (re)assigning the
abirtrator_update_callback_ here.

I'm surprised that none of the tests caught this, since it'll mean that the
callback gets called with the LocationProvider pointer set to the wrong value.

I'm also surprised that things compile as-is, since OnLocationUpdate is still
calling arbitrator_update_callback_ with a single parameter (i.e. it's failing
to pass the LocationProvider parameter!)

[CJ, 2016/08/22 17:41:31]

Done.

+}
+
 scoped_refptr<AccessTokenStore> LocationArbitratorImpl::NewAccessTokenStore() {
   return delegate_->CreateAccessTokenStore();
 }
@@ -205,7 +220,7 @@ bool LocationArbitratorImpl::IsNewPositionBetter(
   return false;
 }
 
-bool LocationArbitratorImpl::HasPermissionBeenGranted() const {
+bool LocationArbitratorImpl::HasPermissionBeenGrantedForTest() const {

[Wez, 2016/08/19 01:33:11]

nit: Move this impl to the equivalent position in this file to match its new
position in the class header, i.e. just after the Default URL stuff.

[CJ, 2016/08/22 17:41:31]

Done.

   return is_permission_granted_;
 }