Refactor to make BlimpLocationProvider accessible to content layer.

content/browser/geolocation/location_arbitrator_impl.cc

Index: content/browser/geolocation/location_arbitrator_impl.cc
diff --git a/content/browser/geolocation/location_arbitrator_impl.cc b/content/browser/geolocation/location_arbitrator_impl.cc
index b9dc791ac27c184d9d91fc510cff18083aca20fb..8eb46124f9a7e9092c1167a6bd14203b6bfc5a62 100644
--- a/content/browser/geolocation/location_arbitrator_impl.cc
+++ b/content/browser/geolocation/location_arbitrator_impl.cc
@@ -52,25 +52,29 @@ void LocationArbitratorImpl::OnPermissionGranted() {
 }
 
 void LocationArbitratorImpl::StartProviders(bool enable_high_accuracy) {
-  // GetAccessTokenStore() will return NULL for embedders not implementing
-  // the AccessTokenStore class, so we report an error to avoid JavaScript
-  // requests of location to wait eternally for a reply.
-  AccessTokenStore* access_token_store = GetAccessTokenStore();
-  if (!access_token_store) {
-    Geoposition position;
-    position.error_code = Geoposition::ERROR_CODE_PERMISSION_DENIED;
-    arbitrator_update_callback_.Run(position);
-    return;
-  }
-
   // Stash options as OnAccessTokenStoresLoaded has not yet been called.
   is_running_ = true;
   enable_high_accuracy_ = enable_high_accuracy;
+
   if (providers_.empty()) {
-    DCHECK(DefaultNetworkProviderURL().is_valid());
-    access_token_store->LoadAccessTokens(
-        base::Bind(&LocationArbitratorImpl::OnAccessTokenStoresLoaded,
-                   base::Unretained(this)));
+    if (GetContentClient()->browser()->UseNetworkLocationProviders()) {
+      // GetAccessTokenStore() will return NULL for embedders not implementing
+      // the AccessTokenStore class, so we report an error to avoid JavaScript
+      // requests of location to wait eternally for a reply.
+      AccessTokenStore* access_token_store = GetAccessTokenStore();
+      if (!access_token_store) {
+        Geoposition position;
+        position.error_code = Geoposition::ERROR_CODE_PERMISSION_DENIED;
+        arbitrator_update_callback_.Run(position);
+        return;

[Kevin M, 2016/06/09 20:36:44]

What about loading the system providers instead of aborting completely?

[CJ, 2016/06/09 23:07:06]

Done.

+      }
+      DCHECK(DefaultNetworkProviderURL().is_valid());
+      access_token_store->LoadAccessTokens(
+          base::Bind(&LocationArbitratorImpl::OnAccessTokenStoresLoaded,
+                     base::Unretained(this)));
+    } else {

[Kevin M, 2016/06/09 20:36:44]

Should this be an "else"? Aren't the network location providers loaded in
addition to the system provider?

[CJ, 2016/06/09 23:07:06]

Done.

+      RegisterSystemProvider();
+    }
   } else {
     DoStartProviders();
   }
@@ -92,6 +96,15 @@ void LocationArbitratorImpl::StopProviders() {
   is_running_ = false;
 }
 
+void LocationArbitratorImpl::RegisterSystemProvider() {

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

Please keep the same order as in the header, so this method goes below
RegisterProvider.

[CJ, 2016/06/09 20:31:50]

Done.

+  LocationProvider* provider =
+      GetContentClient()->browser()->OverrideSystemLocationProvider();
+  if (!provider)
+    provider = NewSystemLocationProvider();
+  RegisterProvider(provider);
+  DoStartProviders();

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

I don't think DoStartProviders should be called form a method called
RegisterSystemProvider, it is unexpected and does not match the behavior of
RegisterProvider. You should probably just call DoStartProviders when it is
needed from StartProviders or OnAccessTokenStoresLoaded.

[CJ, 2016/06/09 20:31:50]

Done.

+}
+
 void LocationArbitratorImpl::OnAccessTokenStoresLoaded(
     AccessTokenStore::AccessTokenMap access_token_map,
     net::URLRequestContextGetter* context_getter) {
@@ -100,9 +113,7 @@ void LocationArbitratorImpl::OnAccessTokenStoresLoaded(
     // completed.
     return;
   }
-  LocationProvider* sole_provider =
-      GetContentClient()->browser()->SoleLocationProvider();
-  if (!sole_provider) {
+  if (GetContentClient()->browser()->UseNetworkLocationProviders()) {
     // If there are no access tokens, boot strap it with the default server URL.
     if (access_token_map.empty())
       access_token_map[DefaultNetworkProviderURL()];
@@ -110,16 +121,8 @@ void LocationArbitratorImpl::OnAccessTokenStoresLoaded(
       RegisterProvider(NewNetworkLocationProvider(
           GetAccessTokenStore(), context_getter, entry.first, entry.second));
     }
-
-    LocationProvider* provider =
-        GetContentClient()->browser()->OverrideSystemLocationProvider();
-    if (!provider)
-      provider = NewSystemLocationProvider();
-    RegisterProvider(provider);
-  } else {
-    RegisterProvider(sole_provider);
   }
-  DoStartProviders();
+  RegisterSystemProvider();

[Kevin M, 2016/06/09 20:36:44]

Do we need to register system providers in this call, or can we do it before we
kick off the async token fetch?

[CJ, 2016/06/09 23:07:06]

Done.

 }
 
 void LocationArbitratorImpl::RegisterProvider(