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

device/geolocation/location_arbitrator_impl.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/location_arbitrator_impl.h"
 
 #include <map>
+#include <memory>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "build/build_config.h"
 #include "device/geolocation/access_token_store.h"
 #include "device/geolocation/geolocation_delegate.h"
 #include "device/geolocation/network_location_provider.h"
 #include "url/gurl.h"
 
 namespace device {
 namespace {
 
 const char* kDefaultNetworkProviderUrl =
     "https://www.googleapis.com/geolocation/v1/geolocate";
 }  // 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_t LocationArbitratorImpl::kFixStaleTimeoutMilliseconds =
     11 * base::Time::kMillisecondsPerSecond;
 
 LocationArbitratorImpl::LocationArbitratorImpl(
     const LocationUpdateCallback& callback,

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

Looks like we should get rid of this parameter, since caller should use
SetUpdateCallback() to set the arbitrator_update_callback.

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

Done.

     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) {}
 
 LocationArbitratorImpl::~LocationArbitratorImpl() {}
 
 GURL LocationArbitratorImpl::DefaultNetworkProviderURL() {
   return GURL(kDefaultNetworkProviderUrl);
 }
 
 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;
 
   if (providers_.empty()) {
     RegisterSystemProvider();
 
     const scoped_refptr<AccessTokenStore> access_token_store =
         GetAccessTokenStore();
     if (access_token_store && delegate_->UseNetworkLocationProviders()) {
       DCHECK(DefaultNetworkProviderURL().is_valid());
       token_store_callback_.Reset(
           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.
   position_provider_ = nullptr;
   position_ = Geoposition();
 
   providers_.clear();
   is_running_ = false;
 }
 
@@ skipping 33 matching lines @@
   DCHECK(new_position.Validate() ||
          new_position.error_code != Geoposition::ERROR_CODE_NONE);
   if (!IsNewPositionBetter(position_, new_position,
                            provider == position_provider_))
     return;
   position_provider_ = provider;
   position_ = new_position;
   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();
 }
 
 scoped_refptr<AccessTokenStore> LocationArbitratorImpl::GetAccessTokenStore() {
   if (!access_token_store_)
     access_token_store_ = NewAccessTokenStore();
   return access_token_store_;
 }
 
@@ skipping 45 matching lines @@
       return true;
     } else if ((GetTimeNow() - old_position.timestamp).InMilliseconds() >
                kFixStaleTimeoutMilliseconds) {
       // Existing fix is stale.
       return true;
     }
   }
   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_;
 }
 
 }  // namespace device