Revert of Reland: Refactor DeviceMotionEventPump to use...

Revert of Reland: Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors (patchset #14 id:260001 of https://codereview.chromium.org/2896583005/ )

Reason for revert:
Need to figure out some issues related to this CL on Windows.

To reland, need to add tests on Android and Mac, and fix the issue on Windows.

Original issue's description:
> Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors
>
> //device/generic_sensors already has all the sensors that can be used
> to implement the DeviceOrientation event:
> https://w3c.github.io/deviceorientation/spec-source-orientation.html
>
> Currently, //content/renderer/device_sensors uses sensors from
> //device/sensors as its backend, and this is one of the CLs that refactor
> //content/renderer/device_sensors to use sensors from
> //device/generic_sensor as the backend and removes //device/sensors.
>
> This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor.
>
> The issue page contains the design doc link for this change.
>
> BUG=721427
>
> Review-Url: https://codereview.chromium.org/2896583005
> Cr-Original-Commit-Position: refs/heads/master@{#480934}
> Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab900596a77958fd
> Review-Url: https://codereview.chromium.org/2896583005
> Cr-Commit-Position: refs/heads/master@{#481791}
> Committed: https://chromium.googlesource.com/chromium/src/+/33f72bfe8b9364e82990f92befa041a08d465d3c

TBR=reillyg@chromium.org,timvolodine@chromium.org,jam@chromium.org,scheib@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=721427

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

content/renderer/BUILD.gn

@@ -458,7 +458,6 @@ target(link_target_type, "renderer") {
     "//device/base/synchronization",
     "//device/gamepad/public/cpp:shared_with_blink",
     "//device/gamepad/public/interfaces",
-    "//device/generic_sensor/public/cpp",
     "//device/screen_orientation/public/interfaces",
     "//device/sensors/public/cpp:full",
     "//device/sensors/public/interfaces",

content/renderer/DEPS

@@ -18,8 +18,6 @@ include_rules = [
   "+device/base/synchronization",
   "+device/gamepad/public/cpp",
   "+device/gamepad/public/interfaces",
-  "+device/generic_sensor/public/cpp",
-  "+device/generic_sensor/public/interfaces",
   "+device/screen_orientation/public/interfaces",
   "+device/sensors/public",
   "+device/usb/public",

content/renderer/device_sensors/device_motion_event_pump.h

@@ -6,105 +6,38 @@
 #define CONTENT_RENDERER_DEVICE_SENSORS_DEVICE_MOTION_EVENT_PUMP_H_
 
 #include <memory>
-#include <utility>
-#include <vector>
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/macros.h"
-#include "base/time/time.h"
-#include "base/timer/timer.h"
-#include "content/public/renderer/platform_event_observer.h"
-#include "content/renderer/render_thread_impl.h"
-#include "device/generic_sensor/public/cpp/sensor_reading.h"
-#include "device/generic_sensor/public/interfaces/sensor.mojom.h"
-#include "device/generic_sensor/public/interfaces/sensor_provider.mojom.h"
+#include "content/renderer/device_sensors/device_sensor_event_pump.h"
+#include "content/renderer/shared_memory_seqlock_reader.h"
 #include "device/sensors/public/cpp/motion_data.h"
-#include "mojo/public/cpp/bindings/binding.h"
-#include "third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionListener.h"
+#include "device/sensors/public/interfaces/motion.mojom.h"
+
+namespace blink {
+class WebDeviceMotionListener;
+}
 
 namespace content {
 
+typedef SharedMemorySeqLockReader<device::MotionData>
+    DeviceMotionSharedMemoryReader;
+
 class CONTENT_EXPORT DeviceMotionEventPump
-    : NON_EXPORTED_BASE(
-          public PlatformEventObserver<blink::WebDeviceMotionListener>) {
+    : public DeviceSensorMojoClientMixin<
+          DeviceSensorEventPump<blink::WebDeviceMotionListener>,
+          device::mojom::MotionSensor> {
  public:
   explicit DeviceMotionEventPump(RenderThread* thread);
   ~DeviceMotionEventPump() override;
 
-  // PlatformEventObserver:
-  void Start(blink::WebPlatformEventListener* listener) override;
-  void Stop() override;
-  void SendStartMessage() override;
-  void SendStopMessage() override;
+  // PlatformEventObserver.
   void SendFakeDataForTesting(void* fake_data) override;
 
  protected:
-  // Default rate for firing events.
-  static constexpr int kDefaultPumpFrequencyHz = 60;
-  static constexpr int kDefaultPumpDelayMicroseconds =
-      base::Time::kMicrosecondsPerSecond / kDefaultPumpFrequencyHz;
-
-  struct CONTENT_EXPORT SensorEntry : public device::mojom::SensorClient {
-    SensorEntry(DeviceMotionEventPump* pump,
-                device::mojom::SensorType sensor_type);
-    ~SensorEntry() override;
-
-    // device::mojom::SensorClient:
-    void RaiseError() override;
-    void SensorReadingChanged() override;
-
-    // Mojo callback for SensorProvider::GetSensor().
-    void OnSensorCreated(device::mojom::SensorInitParamsPtr params,
-                         device::mojom::SensorClientRequest client_request);
-
-    // Mojo callback for Sensor::AddConfiguration().
-    void OnSensorAddConfiguration(bool success);
-
-    void HandleSensorError();
-
-    bool SensorReadingCouldBeRead();
-
-    DeviceMotionEventPump* event_pump;
-    device::mojom::SensorPtr sensor;
-    device::mojom::SensorType type;
-    device::mojom::ReportingMode mode;
-    device::PlatformSensorConfiguration default_config;
-    mojo::ScopedSharedBufferHandle shared_buffer_handle;
-    mojo::ScopedSharedBufferMapping shared_buffer;
-    device::SensorReading reading;
-    mojo::Binding<device::mojom::SensorClient> client_binding;
-  };
-
-  friend struct SensorEntry;
-
-  virtual void FireEvent();
-
-  void DidStart();
-
-  SensorEntry accelerometer_;
-  SensorEntry linear_acceleration_sensor_;
-  SensorEntry gyroscope_;
-
- private:
-  // TODO(juncai): refactor DeviceMotionEventPump to use DeviceSensorEventPump
-  // when refactoring DeviceOrientation.
-  //
-  // The pump is a tri-state automaton with allowed transitions as follows:
-  // STOPPED -> PENDING_START
-  // PENDING_START -> RUNNING
-  // PENDING_START -> STOPPED
-  // RUNNING -> STOPPED
-  enum class PumpState { STOPPED, RUNNING, PENDING_START };
-
-  bool CanStart() const;
-  void GetDataFromSharedMemory(device::MotionData* data);
-  void GetSensor(SensorEntry* sensor_entry);
-  void HandleSensorProviderError();
+  void FireEvent() override;
+  bool InitializeReader(base::SharedMemoryHandle handle) override;
 
-  mojo::InterfacePtr<device::mojom::SensorProvider> sensor_provider_;
-  PumpState state_;
-  base::RepeatingTimer timer_;
+  std::unique_ptr<DeviceMotionSharedMemoryReader> reader_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceMotionEventPump);
 };

content/test/BUILD.gn

@@ -1532,7 +1532,6 @@ test("content_unittests") {
     "//device/gamepad",
     "//device/gamepad:test_helpers",
     "//device/gamepad/public/cpp:shared_with_blink",
-    "//device/generic_sensor/public/cpp",
     "//device/sensors/public/cpp:full",
     "//device/sensors/public/interfaces",
     "//gin",

content/test/DEPS

@@ -12,7 +12,6 @@ include_rules = [
   "+content",
   "+device/bluetooth", # For WebBluetooth tests
   "+device/gamepad/public/cpp",
-  "+device/generic_sensor/public/cpp",
   "+device/sensors/public/cpp",
   # For loading V8's initial snapshot from external files.
   "+gin/v8_initializer.h",

content/test/data/device_sensors/device_motion_only_some_sensors_are_available_test.html

-<html>
-  <head>
-    <title>DeviceMotion only some sensors are available test</title>
-    <script type="text/javascript">
-      let expectedInterval = Math.floor(1000 / 60);
-      function checkMotionEvent(event) {
-        return event.acceleration.x == 1 &&
-               event.acceleration.y == 2 &&
-               event.acceleration.z == 3 &&
-               event.accelerationIncludingGravity.x == null &&
-               event.accelerationIncludingGravity.y == null &&
-               event.accelerationIncludingGravity.z == null &&
-               event.rotationRate.alpha == 7 &&
-               event.rotationRate.beta == 8 &&
-               event.rotationRate.gamma == 9 &&
-               event.interval == expectedInterval;
-      }
-
-      function onMotion(event) {
-        if (checkMotionEvent(event)) {
-          window.removeEventListener('devicemotion', onMotion);
-          pass();
-        } else {
-          fail();
-        }
-      }
-
-      function pass() {
-        document.getElementById('status').innerHTML = 'PASS';
-        document.location = '#pass';
-      }
-
-      function fail() {
-        document.location = '#fail';
-      }
-    </script>
-  </head>
-  <body onLoad="window.addEventListener('devicemotion', onMotion)">
-    <div id="status">FAIL</div>
-  </body>
-</html>

content/test/data/device_sensors/device_motion_test.html

@@ -2,7 +2,6 @@
   <head>
     <title>DeviceMotion test</title>
     <script type="text/javascript">
-      let expectedInterval = Math.floor(1000 / 60);
       function checkMotionEvent(event) {
         return event.acceleration.x == 1 &&
                event.acceleration.y == 2 &&
@@ -13,7 +12,7 @@
                event.rotationRate.alpha == 7 &&
                event.rotationRate.beta == 8 &&
                event.rotationRate.gamma == 9 &&
-               event.interval == expectedInterval;
+               event.interval == 100;
       }
 
       function onMotion(event) {

device/generic_sensor/BUILD.gn

@@ -52,7 +52,6 @@ component("generic_sensor") {
   deps = [
     "//base",
     "//device/base/synchronization",
-    "//services/device/public/cpp:device_features",
   ]
 
   public_deps = [

device/generic_sensor/DEPS

 include_rules = [
   "+device/base/synchronization",
   "+jni",
-  "+services/device/public/cpp/device_features.h",
   "+third_party/sudden_motion_sensor",
 ]

device/generic_sensor/platform_sensor_provider_android.cc

@@ -36,10 +36,8 @@ void PlatformSensorProviderAndroid::CreateSensorInternal(
   ScopedJavaLocalRef<jobject> sensor = Java_PlatformSensorProvider_createSensor(
       env, j_object_.obj(), static_cast<jint>(type));
 
-  if (!sensor.obj()) {
+  if (!sensor.obj())
     callback.Run(nullptr);
-    return;
-  }
 
   scoped_refptr<PlatformSensorAndroid> concrete_sensor =
       new PlatformSensorAndroid(type, std::move(mapping), this, sensor);

device/generic_sensor/sensor_provider_impl.cc

@@ -6,13 +6,11 @@
 
 #include <utility>
 
-#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "device/generic_sensor/platform_sensor_provider.h"
 #include "device/generic_sensor/sensor_impl.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
-#include "services/device/public/cpp/device_features.h"
 
 namespace device {
 
@@ -56,17 +54,6 @@ SensorProviderImpl::~SensorProviderImpl() {}
 void SensorProviderImpl::GetSensor(mojom::SensorType type,
                                    mojom::SensorRequest sensor_request,
                                    GetSensorCallback callback) {
-  // TODO(juncai): remove when the GenericSensor feature goes stable.
-  // For sensors that are used by DeviceMotionEvent, don't check the
-  // features::kGenericSensor flag.
-  if (!base::FeatureList::IsEnabled(features::kGenericSensor) &&
-      !(type == mojom::SensorType::ACCELEROMETER ||
-        type == mojom::SensorType::LINEAR_ACCELERATION ||
-        type == mojom::SensorType::GYROSCOPE)) {
-    NotifySensorCreated(nullptr, nullptr, std::move(callback));
-    return;
-  }
-
   auto cloned_handle = provider_->CloneSharedBufferHandle();
   if (!cloned_handle.is_valid()) {
     NotifySensorCreated(nullptr, nullptr, std::move(callback));

services/device/device_service.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
@@ -17,6 +18,7 @@
 #include "mojo/public/cpp/system/message_pipe.h"
 #include "services/device/fingerprint/fingerprint.h"
 #include "services/device/power_monitor/power_monitor_message_broadcaster.h"
+#include "services/device/public/cpp/device_features.h"
 #include "services/device/public/interfaces/battery_monitor.mojom.h"
 #include "services/device/time_zone_monitor/time_zone_monitor.h"
 #include "services/service_manager/public/cpp/bind_source_info.h"
@@ -88,6 +90,8 @@ DeviceService::~DeviceService() {
 void DeviceService::OnStart() {
   registry_.AddInterface<mojom::Fingerprint>(base::Bind(
       &DeviceService::BindFingerprintRequest, base::Unretained(this)));
+  registry_.AddInterface<mojom::MotionSensor>(base::Bind(
+      &DeviceService::BindMotionSensorRequest, base::Unretained(this)));
   registry_.AddInterface<mojom::OrientationSensor>(base::Bind(
       &DeviceService::BindOrientationSensorRequest, base::Unretained(this)));
   registry_.AddInterface<mojom::OrientationAbsoluteSensor>(
@@ -98,8 +102,10 @@ void DeviceService::OnStart() {
   registry_.AddInterface<mojom::ScreenOrientationListener>(
       base::Bind(&DeviceService::BindScreenOrientationListenerRequest,
                  base::Unretained(this)));
+  if (base::FeatureList::IsEnabled(features::kGenericSensor)) {
     registry_.AddInterface<mojom::SensorProvider>(base::Bind(
         &DeviceService::BindSensorProviderRequest, base::Unretained(this)));
+  }
   registry_.AddInterface<mojom::TimeZoneMonitor>(base::Bind(
       &DeviceService::BindTimeZoneMonitorRequest, base::Unretained(this)));
   registry_.AddInterface<mojom::WakeLockProvider>(base::Bind(
@@ -158,6 +164,23 @@ void DeviceService::BindFingerprintRequest(
   Fingerprint::Create(std::move(request));
 }
 
+void DeviceService::BindMotionSensorRequest(
+    const service_manager::BindSourceInfo& source_info,
+    mojom::MotionSensorRequest request) {
+#if defined(OS_ANDROID)
+  // On Android the device sensors implementations need to run on the UI thread
+  // to communicate to Java.
+  DeviceMotionHost::Create(std::move(request));
+#else
+  // On platforms other than Android the device sensors implementations run on
+  // the IO thread.
+  if (io_task_runner_) {
+    io_task_runner_->PostTask(FROM_HERE, base::Bind(&DeviceMotionHost::Create,
+                                                    base::Passed(&request)));
+  }
+#endif  // defined(OS_ANDROID)
+}
+
 void DeviceService::BindOrientationSensorRequest(
     const service_manager::BindSourceInfo& source_info,
     mojom::OrientationSensorRequest request) {

services/device/device_service.h

@@ -8,6 +8,7 @@
 #include "base/memory/ref_counted.h"
 #include "device/generic_sensor/public/interfaces/sensor_provider.mojom.h"
 #include "device/screen_orientation/public/interfaces/screen_orientation.mojom.h"
+#include "device/sensors/public/interfaces/motion.mojom.h"
 #include "device/sensors/public/interfaces/orientation.mojom.h"
 #include "device/wake_lock/public/interfaces/wake_lock_provider.mojom.h"
 #include "device/wake_lock/wake_lock_context.h"
@@ -74,6 +75,10 @@ class DeviceService : public service_manager::Service {
       const service_manager::BindSourceInfo& source_info,
       mojom::FingerprintRequest request);
 
+  void BindMotionSensorRequest(
+      const service_manager::BindSourceInfo& source_info,
+      mojom::MotionSensorRequest request);
+
   void BindOrientationSensorRequest(
       const service_manager::BindSourceInfo& source_info,
       mojom::OrientationSensorRequest request);