Refactor to make BlimpLocationProvider accessible to content layer.

This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers.

BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location.

Other additions:
* Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects.
* Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false.

BUG=614486

Review-Url: https://codereview.chromium.org/2028823002
Cr-Commit-Position: refs/heads/master@{#401453}

blimp/engine/BUILD.gn

@@ -69,6 +69,7 @@ source_set("app") {
     ":blob_channel_service",
     ":common",
     ":crash",
+    ":feature",
     ":renderer",
     ":session",
     "//base",

blimp/engine/app/blimp_content_browser_client.cc

@@ -39,6 +39,18 @@ BlimpBrowserContext* BlimpContentBrowserClient::GetBrowserContext() {
   return blimp_browser_main_parts_->GetBrowserContext();
 }
 
+content::LocationProvider*
+BlimpContentBrowserClient::OverrideSystemLocationProvider() {
+  if (!location_provider_) {
+    location_provider_ = base::WrapUnique(new BlimpLocationProvider());
+  }
+  return location_provider_.get();
+}
+
+bool BlimpContentBrowserClient::UseNetworkLocationProviders() {
+  return false;
+}
+
 void BlimpContentBrowserClient::RegisterRenderProcessMojoServices(
     content::ServiceRegistry* registry,
     content::RenderProcessHost* render_process_host) {

blimp/engine/app/blimp_content_browser_client.h

@@ -6,6 +6,7 @@
 #define BLIMP_ENGINE_APP_BLIMP_CONTENT_BROWSER_CLIENT_H_
 
 #include "base/macros.h"
+#include "blimp/engine/feature/geolocation/blimp_location_provider.h"
 #include "content/public/browser/content_browser_client.h"
 
 namespace blimp {
@@ -32,12 +33,17 @@ class BlimpContentBrowserClient : public content::ContentBrowserClient {
       content::ServiceRegistry* registry,
       content::RenderProcessHost* render_process_host) override;
 
+  content::LocationProvider* OverrideSystemLocationProvider() override;
+  bool UseNetworkLocationProviders() override;
+
   BlimpBrowserContext* GetBrowserContext();
 
  private:
   // Owned by BrowserMainLoop
   BlimpBrowserMainParts* blimp_browser_main_parts_ = nullptr;
 
+  std::unique_ptr<BlimpLocationProvider> location_provider_;
+
   DISALLOW_COPY_AND_ASSIGN(BlimpContentBrowserClient);
 };
 

blimp/engine/feature/geolocation/blimp_location_provider.cc

@@ -9,9 +9,7 @@
 namespace blimp {
 namespace engine {
 
-BlimpLocationProvider::BlimpLocationProvider() {
-  NOTIMPLEMENTED();
-}
+BlimpLocationProvider::BlimpLocationProvider() {}
 
 BlimpLocationProvider::~BlimpLocationProvider() {
   StopProvider();
@@ -23,7 +21,6 @@ bool BlimpLocationProvider::StartProvider(bool high_accuracy) {
 }
 
 void BlimpLocationProvider::StopProvider() {
-  NOTIMPLEMENTED();
 }
 
 void BlimpLocationProvider::GetPosition(content::Geoposition* position) {

content/browser/geolocation/location_arbitrator_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/memory/ptr_util.h"
 #include "build/build_config.h"
 #include "content/browser/geolocation/network_location_provider.h"
 #include "content/public/browser/access_token_store.h"
@@ -52,31 +53,36 @@ 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)));
-  } else {
-    DoStartProviders();
+    RegisterSystemProvider();
+
+    AccessTokenStore* access_token_store = GetAccessTokenStore();
+    if (access_token_store &&
+        GetContentClient()->browser()->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;
+    }
   }
+  DoStartProviders();
 }
 
 void 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;
+  }
   for (const auto& provider : providers_)
     provider->StartProvider(enable_high_accuracy_);
 }
@@ -95,35 +101,32 @@ void LocationArbitratorImpl::StopProviders() {
 void LocationArbitratorImpl::OnAccessTokenStoresLoaded(
     AccessTokenStore::AccessTokenMap access_token_map,
     net::URLRequestContextGetter* context_getter) {
-  if (!is_running_ || !providers_.empty()) {
-    // A second StartProviders() call may have arrived before the first
-    // completed.
-    return;
-  }
   // If there are no access tokens, boot strap it with the default server URL.
   if (access_token_map.empty())
     access_token_map[DefaultNetworkProviderURL()];
   for (const auto& entry : access_token_map) {
     RegisterProvider(NewNetworkLocationProvider(
         GetAccessTokenStore(), context_getter, entry.first, entry.second));
   }
-
-  LocationProvider* provider =
-      GetContentClient()->browser()->OverrideSystemLocationProvider();
-  if (!provider)
-    provider = NewSystemLocationProvider();
-  RegisterProvider(provider);
   DoStartProviders();
 }
 
 void LocationArbitratorImpl::RegisterProvider(
-    LocationProvider* provider) {
+    std::unique_ptr<LocationProvider> provider) {
   if (!provider)
     return;
   provider->SetUpdateCallback(provider_update_callback_);
   if (is_permission_granted_)
     provider->OnPermissionGranted();
-  providers_.push_back(provider);
+  providers_.push_back(std::move(provider));
+}
+
+void LocationArbitratorImpl::RegisterSystemProvider() {
+  std::unique_ptr<LocationProvider> provider = base::WrapUnique(
+      GetContentClient()->browser()->OverrideSystemLocationProvider());
+  if (!provider)
+    provider = NewSystemLocationProvider();
+  RegisterProvider(std::move(provider));
 }
 
 void LocationArbitratorImpl::OnLocationUpdate(const LocationProvider* provider,
@@ -148,7 +151,8 @@ AccessTokenStore* LocationArbitratorImpl::GetAccessTokenStore() {
   return access_token_store_.get();
 }
 
-LocationProvider* LocationArbitratorImpl::NewNetworkLocationProvider(
+std::unique_ptr<LocationProvider>
+LocationArbitratorImpl::NewNetworkLocationProvider(
     AccessTokenStore* access_token_store,
     net::URLRequestContextGetter* context,
     const GURL& url,
@@ -157,16 +161,17 @@ LocationProvider* LocationArbitratorImpl::NewNetworkLocationProvider(
   // Android uses its own SystemLocationProvider.
   return NULL;
 #else
-  return new NetworkLocationProvider(access_token_store, context, url,
-                                     access_token);
+  return base::WrapUnique(new NetworkLocationProvider(
+      access_token_store, context, url, access_token));
 #endif
 }
 
-LocationProvider* LocationArbitratorImpl::NewSystemLocationProvider() {
+std::unique_ptr<LocationProvider>
+LocationArbitratorImpl::NewSystemLocationProvider() {
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX)
   return NULL;
 #else
-  return content::NewSystemLocationProvider();
+  return base::WrapUnique(content::NewSystemLocationProvider());
 #endif
 }
 

content/browser/geolocation/location_arbitrator_impl.h

@@ -6,8 +6,10 @@
 #define CONTENT_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_IMPL_H_
 
 #include <stdint.h>
+#include <vector>
 
 #include "base/callback_forward.h"
+#include "base/cancelable_callback.h"
 #include "base/macros.h"
 #include "base/memory/scoped_vector.h"
 #include "base/strings/string16.h"
@@ -56,18 +58,20 @@ class CONTENT_EXPORT LocationArbitratorImpl : public LocationArbitrator {
   // These functions are useful for injection of dependencies in derived
   // testing classes.
   virtual AccessTokenStore* NewAccessTokenStore();
-  virtual LocationProvider* NewNetworkLocationProvider(
+  virtual std::unique_ptr<LocationProvider> NewNetworkLocationProvider(
       AccessTokenStore* access_token_store,
       net::URLRequestContextGetter* context,
       const GURL& url,
       const base::string16& access_token);
-  virtual LocationProvider* NewSystemLocationProvider();
+  virtual std::unique_ptr<LocationProvider> NewSystemLocationProvider();
   virtual base::Time GetTimeNow() const;
 
  private:
-  // Takes ownership of |provider| on entry; it will either be added to
-  // |providers_| or deleted on error (e.g. it fails to start).
-  void RegisterProvider(LocationProvider* provider);
+  // Provider will either be added to |providers_| or
+  // deleted on error (e.g. it fails to start).
+  void RegisterProvider(std::unique_ptr<LocationProvider> provider);
+
+  void RegisterSystemProvider();
   void OnAccessTokenStoresLoaded(
       AccessTokenStore::AccessTokenMap access_token_map,
       net::URLRequestContextGetter* context_getter);
@@ -87,7 +91,13 @@ class CONTENT_EXPORT LocationArbitratorImpl : public LocationArbitrator {
   scoped_refptr<AccessTokenStore> access_token_store_;
   LocationUpdateCallback arbitrator_update_callback_;
   LocationProvider::LocationProviderUpdateCallback provider_update_callback_;
-  ScopedVector<LocationProvider> providers_;
+
+  // The CancelableCallback will prevent OnAccessTokenStoresLoaded from being
+  // called multiple times by calling Reset() at the time of binding.
+  base::CancelableCallback<void(AccessTokenStore::AccessTokenMap,
+                                net::URLRequestContextGetter*)>
+      token_store_callback_;
+  std::vector<std::unique_ptr<LocationProvider>> providers_;
   bool enable_high_accuracy_;
   // The provider which supplied the current |position_|
   const LocationProvider* position_provider_;