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..d83cd27a114efa0fd1ab4a34cc8f13f1f0b626ea 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,10 +69,11 @@ 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();

[Wez, 2016/08/09 01:17:04]

Looks like it might be worthwhile to have DoStartProviders return a bool
indicating whether providers_ is empty, and to return that here.

(Reason being that if there is no system provider and the caller asks for no
network providers then providers_ is empty, so we should probably fail :)

[CJ, 2016/08/11 22:06:43]

Done.

+  return true;
 }
 
 void LocationArbitratorImpl::DoStartProviders() {
@@ -86,7 +89,7 @@ void LocationArbitratorImpl::DoStartProviders() {
     provider->StartProvider(enable_high_accuracy_);
 }
 
-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 +143,23 @@ void LocationArbitratorImpl::OnLocationUpdate(const LocationProvider* provider,
   arbitrator_update_callback_.Run(position_);
 }
 
+void LocationArbitratorImpl::RequestRefresh() {
+  if (is_permission_granted_) {
+    for (const auto& provider : providers_)
+      provider->RequestRefresh();

[Wez, 2016/08/09 01:17:04]

nit: Interesting that there are no other RequestRefresh() mentions in
LocationArbitratorImpl - are we sure that anything actually needs it? Perhaps we
could just remove it from the LocationProvider API..?

[CJ, 2016/08/11 22:06:43]

Done.

+  }
+}
+
+void LocationArbitratorImpl::GetPosition(Geoposition* position) {
+  DCHECK(position);

[Wez, 2016/08/09 01:17:04]

nit: No need to DCHECK - you're about to assign to |position|, things will
explode cleanly enough then if it's null.

[CJ, 2016/08/11 22:06:43]

Done.

+  position = &position_;

[Wez, 2016/08/09 01:17:04]

This isn't doing what you intend.

You're setting the function-local variable |position| to point to |position_| -
when GetPosition() returns there will be no observable side-effect.

What you _want_ to do is copy/assign the value of |position_| into the memory
that |position| points to, I suspect. :P

[CJ, 2016/08/11 22:06:43]

Done.

+}
+
+void LocationArbitratorImpl::SetUpdateCallback(
+    const LocationProviderUpdateCallback& callback) {
+  provider_update_callback_ = callback;
+}
+
 scoped_refptr<AccessTokenStore> LocationArbitratorImpl::NewAccessTokenStore() {
   return delegate_->CreateAccessTokenStore();
 }