Reland: Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors

content/renderer/device_sensors/device_motion_event_pump.h

 // Copyright 2014 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.
 
 #ifndef CONTENT_RENDERER_DEVICE_SENSORS_DEVICE_MOTION_EVENT_PUMP_H_
 #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 "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 "device/sensors/public/interfaces/motion.mojom.h"
-
-namespace blink {
-class WebDeviceMotionListener;
-}
+#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 "mojo/public/cpp/bindings/binding.h"
+#include "third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h"
+#include "third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionListener.h"
 
 namespace content {
 
-typedef SharedMemorySeqLockReader<device::MotionData>
-    DeviceMotionSharedMemoryReader;
-
 class CONTENT_EXPORT DeviceMotionEventPump

[timvolodine, 2017/05/23 12:33:40]

this implementation looks much more complicated now (and much larger as well).
and the orientation side is not even covered in this patch. can this be
simplified to have a simpler structure as at was before?

[juncai, 2017/05/23 21:33:55]

This implementation contains some common code that the orientation side can use
too, for example, the struct SensorEntry can be used by both. The original CL
that does the change for both motion and orientation is:
https://codereview.chromium.org/2885203004/
The above CL is closed in favor of several smaller CLs, and this CL is one of
them.

And I am going to have a follow-up CL for refactoring device orientation. And
after all these refactoring is done, the code will be much simplified.

[timvolodine, 2017/05/24 15:30:16]

Thanks for the pointer, I'll have a look. Just not sure if it may be easier to
provide some more support on the back-end side for this. There is quite some
logic for the DeviceOrientation implementation in DeviceSensors.java (and its
test) which would need to be accommodated somehow, are there plans for doing
that? (and how?)

[juncai, 2017/05/26 02:38:53]

Yes, there are plans to do that and the idea is similar to the closed CL:
https://codereview.chromium.org/2885203004

From what I understand, the implementation logic in DeviceSensors.java is to use
six kinds of sensor to gather motion/orientation data:
https://cs.chromium.org/chromium/src/device/sensors/android/java/src/org/chro...
And the //device/generic_sensor/ already provides all the sensors that are
needed to implement the device motion/orientation event. Those are abstracted
into the sensor.mojom interfaces. And for orientation there is a 3-way fallback
approach which is implemented as:
https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device...

And there are test code to cover the case when different sensors are available:
https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device...

-    : public DeviceSensorMojoClientMixin<
-          DeviceSensorEventPump<blink::WebDeviceMotionListener>,
-          device::mojom::MotionSensor> {
+    : NON_EXPORTED_BASE(
+          public PlatformEventObserver<blink::WebDeviceMotionListener>) {
  public:
   explicit DeviceMotionEventPump(RenderThread* thread);
   ~DeviceMotionEventPump() override;
 
-  // PlatformEventObserver.
+  // PlatformEventObserver:
+  void Start(blink::WebPlatformEventListener* listener) override;
+  void Stop() override;
+  void SendStartMessage() override;
+  void SendStopMessage() override;
   void SendFakeDataForTesting(void* fake_data) override;
 
  protected:
-  void FireEvent() override;
-  bool InitializeReader(base::SharedMemoryHandle handle) override;
+  // Default rate for firing events.
+  static constexpr int kDefaultPumpFrequencyHz = 60;
+  static constexpr int kDefaultPumpDelayMicroseconds =
+      base::Time::kMicrosecondsPerSecond / kDefaultPumpFrequencyHz;
 
-  std::unique_ptr<DeviceMotionSharedMemoryReader> reader_;
+  static constexpr int kMaxReadAttemptsCount = 10;
+
+  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();
+
+    // Updates sensor reading from shared buffer.
+    bool UpdateSensorReading();
+
+    bool TryReadFromBuffer(device::SensorReading* result);
+
+    bool SensorReadingUpdated();
+
+    DeviceMotionEventPump* event_pump;

[timvolodine, 2017/05/24 15:30:16]

we normally use _ for members, e.g. event_pump_?

[juncai, 2017/05/26 02:38:53]

This is inside a struct and it doesn't need the trailing underscore according to
the Google C++ coding style:
https://google.github.io/styleguide/cppguide.html#Variable_Names

+    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;
+    std::pair<double, double> frequency_limits;
+    mojo::Binding<device::mojom::SensorClient> client_binding;
+  };
+
+  friend struct SensorEntry;
+
+  virtual void FireEvent();
+
+  void DidStart();
+
+  SensorEntry accelerometer_sensor_;
+  SensorEntry linear_acceleration_sensor_;
+  SensorEntry gyroscope_sensor_;
+  int pump_delay_microseconds_;
+
+ private:
+  // 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;
+  bool GetDataFromSharedMemory(blink::WebDeviceMotionData* data);
+  void GetSensor(SensorEntry* sensor_entry);
+  void HandleSensorProviderError();
+
+  mojo::InterfacePtr<device::mojom::SensorProvider> sensor_provider_;
+  // The number of sensors that are tried by calling
+  // SensorProvider::GetSensor() and Sensor::AddConfiguration().
+  int num_sensors_tried_;
+  PumpState state_;
+  base::RepeatingTimer timer_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceMotionEventPump);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_DEVICE_SENSORS_DEVICE_MOTION_EVENT_PUMP_H_