Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the BlimpClientSession, allows the Client-side Geolocation Feature to communicate with the LocationArbitrator, and makes the necessary changes to permissions. Additional Changes: * Renames LocationArbitratorImpl -> LocationArbitrator BUG=614486, 641071 Review-Url: https://codereview.chromium.org/2249283003 Cr-Commit-Position: refs/heads/master@{#416190}
FairyWorld · Sep 2, 2016 · 8e0b2a7 · 8e0b2a7
1 parent b9ca6b6
commit 8e0b2a7
Show file tree

Hide file tree

Showing 23 changed files with 144 additions and 86 deletions.
diff --git a/blimp/client/BUILD.gn b/blimp/client/BUILD.gn
@@ -18,7 +18,9 @@ source_set("session") {
   public_deps = [
     "//blimp/client/core",
     "//blimp/client/core:switches",
+    "//blimp/client/core/geolocation",
     "//blimp/common/proto",
+    "//device/geolocation:device_geolocation",
     "//ui/events",
   ]
 

diff --git a/blimp/client/DEPS b/blimp/client/DEPS
@@ -8,6 +8,7 @@ include_rules = [
   "+components/version_info",
   "+components/sync/android",
   "-content",
+  "+device/geolocation",
   "+google_apis/gaia",
   "+gpu",
   "+jni",

diff --git a/blimp/client/app/android/AndroidManifest.xml.jinja2 b/blimp/client/app/android/AndroidManifest.xml.jinja2
@@ -7,6 +7,7 @@
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
         package="{{manifest_package}}">
     <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="21" />
+    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>
     <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
     <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
     <uses-permission android:name="android.permission.INTERNET"/>

diff --git a/blimp/client/core/BUILD.gn b/blimp/client/core/BUILD.gn
@@ -102,11 +102,13 @@ source_set("context") {
     "//blimp/client/core/contents",
     "//blimp/client/core/session",
     "//blimp/client/public:public_headers",
+    "//device/geolocation:device_geolocation",
     "//url",
   ]
 
   deps = [
     ":switches",
+    "//blimp/client/core/geolocation",
     "//blimp/client/core/settings",
   ]
 
@@ -167,6 +169,7 @@ if (is_android) {
       ":switches_java",
       "//blimp/client/core/contents:contents_java",
       "//blimp/client/core/settings:settings_java",
+      "//device/geolocation:geolocation_java",
     ]
   }
 

diff --git a/blimp/client/core/blimp_client_context_impl.cc b/blimp/client/core/blimp_client_context_impl.cc
@@ -4,6 +4,8 @@
 
 #include "blimp/client/core/blimp_client_context_impl.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
@@ -16,11 +18,14 @@
 #include "blimp/client/core/contents/ime_feature.h"
 #include "blimp/client/core/contents/navigation_feature.h"
 #include "blimp/client/core/contents/tab_control_feature.h"
+#include "blimp/client/core/geolocation/geolocation_feature.h"
 #include "blimp/client/core/render_widget/render_widget_feature.h"
 #include "blimp/client/core/session/cross_thread_network_event_observer.h"
 #include "blimp/client/core/settings/settings_feature.h"
 #include "blimp/client/public/blimp_client_context_delegate.h"
 #include "blimp/client/public/compositor/compositor_dependencies.h"
+#include "device/geolocation/geolocation_delegate.h"
+#include "device/geolocation/location_arbitrator.h"
 #include "ui/gfx/native_widget_types.h"
 
 #if defined(OS_ANDROID)
@@ -65,6 +70,9 @@ BlimpClientContextImpl::BlimpClientContextImpl(
       blimp_compositor_dependencies_(
           base::MakeUnique<BlimpCompositorDependencies>(
               std::move(compositor_dependencies))),
+      geolocation_feature_(base::MakeUnique<GeolocationFeature>(
+          base::MakeUnique<device::LocationArbitrator>(
+              base::MakeUnique<device::GeolocationDelegate>()))),
       ime_feature_(new ImeFeature),
       navigation_feature_(new NavigationFeature),
       render_widget_feature_(new RenderWidgetFeature),
@@ -89,7 +97,7 @@ BlimpClientContextImpl::BlimpClientContextImpl(
   RegisterFeatures();
   InitializeSettings();
 
-  // Initialize must only be posted after the calls features have been
+  // Initialize must only be posted after the features have been
   // registered.
   io_thread_task_runner_->PostTask(
       FROM_HERE, base::Bind(&ClientNetworkComponents::Initialize,
@@ -170,6 +178,9 @@ void BlimpClientContextImpl::ConnectWithAssignment(
 
 void BlimpClientContextImpl::RegisterFeatures() {
   // Register features' message senders and receivers.
+  geolocation_feature_->set_outgoing_message_processor(
+      thread_pipe_manager_->RegisterFeature(BlimpMessage::kGeolocation,
+                                            geolocation_feature_.get()));
   ime_feature_->set_outgoing_message_processor(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kIme,
                                             ime_feature_.get()));

diff --git a/blimp/client/core/blimp_client_context_impl.h b/blimp/client/core/blimp_client_context_impl.h
@@ -6,6 +6,7 @@
 #define BLIMP_CLIENT_CORE_BLIMP_CLIENT_CONTEXT_IMPL_H_
 
 #include <memory>
+#include <string>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -27,6 +28,7 @@ namespace client {
 class BlimpCompositorDependencies;
 class BlimpContentsManager;
 class CompositorDependencies;
+class GeolocationFeature;
 class ImeFeature;
 class NavigationFeature;
 class RenderWidgetFeature;
@@ -98,6 +100,7 @@ class BlimpClientContextImpl : public BlimpClientContext,
   std::unique_ptr<BlimpCompositorDependencies> blimp_compositor_dependencies_;
 
   // Features to handle all incoming and outgoing protobuf messages.
+  std::unique_ptr<GeolocationFeature> geolocation_feature_;
   std::unique_ptr<ImeFeature> ime_feature_;
   std::unique_ptr<NavigationFeature> navigation_feature_;
   std::unique_ptr<RenderWidgetFeature> render_widget_feature_;

diff --git a/blimp/client/core/geolocation/BUILD.gn b/blimp/client/core/geolocation/BUILD.gn
@@ -10,6 +10,7 @@ if (is_android) {
 source_set("geolocation") {
   visibility = [
     "//blimp/client/core/*",
+    "//blimp/client:session",
     "//blimp/engine:browser_tests",
   ]
 

diff --git a/blimp/client/session/blimp_client_session.cc b/blimp/client/session/blimp_client_session.cc
@@ -20,6 +20,7 @@
 #include "blimp/client/core/contents/ime_feature.h"
 #include "blimp/client/core/contents/navigation_feature.h"
 #include "blimp/client/core/contents/tab_control_feature.h"
+#include "blimp/client/core/geolocation/geolocation_feature.h"
 #include "blimp/client/core/render_widget/render_widget_feature.h"
 #include "blimp/client/core/session/client_network_components.h"
 #include "blimp/client/core/session/cross_thread_network_event_observer.h"
@@ -30,13 +31,18 @@
 #include "blimp/net/blob_channel/blob_channel_receiver.h"
 #include "blimp/net/blob_channel/helium_blob_receiver_delegate.h"
 #include "blimp/net/thread_pipe_manager.h"
+#include "device/geolocation/geolocation_delegate.h"
+#include "device/geolocation/location_arbitrator.h"
 #include "url/gurl.h"
 
 namespace blimp {
 namespace client {
 
 BlimpClientSession::BlimpClientSession(const GURL& assigner_endpoint)
     : io_thread_("BlimpIOThread"),
+      geolocation_feature_(base::MakeUnique<GeolocationFeature>(
+          base::MakeUnique<device::LocationArbitrator>(
+              base::MakeUnique<device::GeolocationDelegate>()))),
       tab_control_feature_(new TabControlFeature),
       navigation_feature_(new NavigationFeature),
       ime_feature_(new ImeFeature),
@@ -108,6 +114,9 @@ void BlimpClientSession::RegisterFeatures() {
       io_thread_.task_runner(), net_components_->GetBrowserConnectionHandler());
 
   // Register features' message senders and receivers.
+  geolocation_feature_->set_outgoing_message_processor(
+      thread_pipe_manager_->RegisterFeature(BlimpMessage::kGeolocation,
+                                            geolocation_feature_.get()));
   tab_control_feature_->set_outgoing_message_processor(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kTabControl,
                                             tab_control_feature_.get()));

diff --git a/blimp/client/session/blimp_client_session.h b/blimp/client/session/blimp_client_session.h
@@ -36,6 +36,7 @@ class ThreadPipeManager;
 namespace client {
 
 class ClientNetworkComponents;
+class GeolocationFeature;
 class NavigationFeature;
 class ImeFeature;
 class RenderWidgetFeature;
@@ -103,6 +104,7 @@ class BlimpClientSession
   BlobImageSerializationProcessor blob_image_processor_;
 
   std::unique_ptr<BlobChannelReceiver> blob_receiver_;
+  std::unique_ptr<GeolocationFeature> geolocation_feature_;
   std::unique_ptr<TabControlFeature> tab_control_feature_;
   std::unique_ptr<NavigationFeature> navigation_feature_;
   std::unique_ptr<ImeFeature> ime_feature_;

diff --git a/blimp/engine/BUILD.gn b/blimp/engine/BUILD.gn
@@ -58,6 +58,8 @@ source_set("app") {
 
   public_deps = [
     ":app_metrics",
+    "//device/geolocation:device_geolocation",
+    "//device/geolocation/public/interfaces",
   ]
 
   deps = [

diff --git a/blimp/engine/app/blimp_permission_manager.cc b/blimp/engine/app/blimp_permission_manager.cc
@@ -23,7 +23,13 @@ int BlimpPermissionManager::RequestPermission(
     const GURL& origin,
     bool user_gesture,
     const base::Callback<void(blink::mojom::PermissionStatus)>& callback) {
-  callback.Run(blink::mojom::PermissionStatus::DENIED);
+  if (permission == content::PermissionType::GEOLOCATION) {
+    VLOG(1) << "Geolocation permission granted.";
+    callback.Run(blink::mojom::PermissionStatus::GRANTED);
+  } else {
+    VLOG(1) << "Permission denied.";
+    callback.Run(blink::mojom::PermissionStatus::DENIED);
+  }
   return kNoPendingOperation;
 }
 

diff --git a/blimp/engine/feature/geolocation/engine_geolocation_feature.cc b/blimp/engine/feature/geolocation/engine_geolocation_feature.cc
@@ -9,6 +9,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "blimp/common/create_blimp_message.h"
 #include "blimp/common/proto/blimp_message.pb.h"
 #include "blimp/common/proto/geolocation.pb.h"
@@ -20,6 +21,7 @@
 namespace blimp {
 namespace engine {
 namespace {
+
 class BlimpGeolocationDelegate : public device::GeolocationDelegate {
  public:
   explicit BlimpGeolocationDelegate(
@@ -120,7 +122,8 @@ void EngineGeolocationFeature::ProcessMessage(
 
 void EngineGeolocationFeature::NotifyCallback(
     const device::Geoposition& position) {
-  geoposition_received_callback_.Run(position);
+  callback_task_runner_->PostTask(
+      FROM_HERE, base::Bind(geoposition_received_callback_, position));
 }
 
 void EngineGeolocationFeature::RequestAccuracy(
@@ -151,6 +154,9 @@ void EngineGeolocationFeature::OnPermissionGranted() {
 void EngineGeolocationFeature::SetUpdateCallback(
     const GeopositionReceivedCallback& callback) {
   geoposition_received_callback_ = callback;
+
+  // Set |callback_task_runner_| to run on the current thread.
+  callback_task_runner_ = base::ThreadTaskRunnerHandle::Get();
 }
 
 }  // namespace engine

diff --git a/blimp/engine/feature/geolocation/engine_geolocation_feature.h b/blimp/engine/feature/geolocation/engine_geolocation_feature.h
@@ -47,6 +47,7 @@ class EngineGeolocationFeature : public BlimpMessageProcessor,
 
   std::unique_ptr<BlimpMessageProcessor> outgoing_message_processor_;
   GeopositionReceivedCallback geoposition_received_callback_;
+  scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner_;
   base::WeakPtrFactory<EngineGeolocationFeature> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(EngineGeolocationFeature);

diff --git a/blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc b/blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
@@ -9,6 +9,8 @@
 #include <utility>
 
 #include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "blimp/common/create_blimp_message.h"
 #include "blimp/common/proto/blimp_message.pb.h"
 #include "blimp/common/proto/geolocation.pb.h"
@@ -41,6 +43,7 @@ void SendMockLocationMessage(BlimpMessageProcessor* processor) {
   net::TestCompletionCallback cb;
   processor->ProcessMessage(std::move(message), cb.callback());
   EXPECT_EQ(net::OK, cb.WaitForResult());
+  base::RunLoop().RunUntilIdle();
 }
 
 void SendMockErrorMessage(BlimpMessageProcessor* processor,
@@ -57,6 +60,7 @@ void SendMockErrorMessage(BlimpMessageProcessor* processor,
   net::TestCompletionCallback cb;
   processor->ProcessMessage(std::move(message), cb.callback());
   EXPECT_EQ(net::OK, cb.WaitForResult());
+  base::RunLoop().RunUntilIdle();
 }
 
 void SendMalformedMessage(BlimpMessageProcessor* processor) {
@@ -117,6 +121,10 @@ class EngineGeolocationFeatureTest : public testing::Test {
   // This is a raw pointer to a class that is owned by the GeolocationFeature.
   MockBlimpMessageProcessor* out_processor_;
 
+  // Processes tasks that were posted as a result of processing of incoming
+  // messages.
+  base::MessageLoop message_loop_;
+
   EngineGeolocationFeature feature_;
   std::unique_ptr<device::LocationProvider> location_provider_;
   device::LocationProvider::LocationProviderUpdateCallback mock_callback_;

diff --git a/device/geolocation/BUILD.gn b/device/geolocation/BUILD.gn
@@ -31,8 +31,8 @@ component("device_geolocation") {
     "geoposition.h",
     "location_api_adapter_android.cc",
     "location_api_adapter_android.h",
-    "location_arbitrator_impl.cc",
-    "location_arbitrator_impl.h",
+    "location_arbitrator.cc",
+    "location_arbitrator.h",
     "location_provider.h",
     "location_provider_android.cc",
     "location_provider_android.h",
@@ -169,7 +169,7 @@ source_set("unittests") {
 
   sources = [
     "geolocation_provider_impl_unittest.cc",
-    "location_arbitrator_impl_unittest.cc",
+    "location_arbitrator_unittest.cc",
     "network_location_provider_unittest.cc",
     "wifi_data_provider_chromeos_unittest.cc",
     "wifi_data_provider_common_unittest.cc",

diff --git a/device/geolocation/geolocation_provider_impl.cc b/device/geolocation/geolocation_provider_impl.cc
@@ -17,7 +17,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "device/geolocation/geolocation_delegate.h"
-#include "device/geolocation/location_arbitrator_impl.h"
+#include "device/geolocation/location_arbitrator.h"
 
 namespace device {
 
@@ -190,8 +190,8 @@ void GeolocationProviderImpl::Init() {
     if (!g_delegate.Get())
       g_delegate.Get().reset(new GeolocationDelegate);
 
-    arbitrator_ =
-        base::MakeUnique<LocationArbitratorImpl>(g_delegate.Get().get());
+    arbitrator_ = base::MakeUnique<LocationArbitrator>(
+        base::WrapUnique(g_delegate.Get().get()));
     arbitrator_->SetUpdateCallback(callback);
   }
 }