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

content/renderer/device_sensors/device_motion_event_pump.cc

Index: content/renderer/device_sensors/device_motion_event_pump.cc
diff --git a/content/renderer/device_sensors/device_motion_event_pump.cc b/content/renderer/device_sensors/device_motion_event_pump.cc
index 94d7f3f7b1e5d81f6c5aaef1c2f218348f22cf4d..4e903e12881e70a74b5f8d90c4ba0f3cedd54863 100644
--- a/content/renderer/device_sensors/device_motion_event_pump.cc
+++ b/content/renderer/device_sensors/device_motion_event_pump.cc
@@ -2,38 +2,314 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "device_motion_event_pump.h"
+#include "content/renderer/device_sensors/device_motion_event_pump.h"
 
+#include "base/memory/ptr_util.h"
+#include "content/public/common/service_names.mojom.h"
 #include "content/public/renderer/render_thread.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "services/device/public/interfaces/constants.mojom.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionListener.h"
 
+namespace {
+
+constexpr int kMaxReadAttemptsCount = 10;
+
+bool TryReadFromBuffer(const device::SensorReadingSharedBuffer* buffer,

[timvolodine, 2017/06/07 20:37:36]

These methods and everything related to reading can perhaps move to a dedicated
(potentially templated) reader class similar to SharedMemorySeqLockReader, so
that it could be reused for the orientation implementation. Fine if this happens
in a follow-up, but please add a TODO then.

[juncai, 2017/06/07 23:05:07]

Done.

+                       device::SensorReading* result) {
+  const device::OneWriterSeqLock& seqlock = buffer->seqlock.value();
+  auto version = seqlock.ReadBegin();
+  auto reading_data = buffer->reading;
+  if (seqlock.ReadRetry(version))
+    return false;
+  *result = reading_data;
+  return true;
+}
+
+// Updates sensor reading from shared buffer.
+bool UpdateSensorReading(const device::SensorReadingSharedBuffer* buffer,
+                         device::SensorReading* result) {
+  int read_attempts = 0;
+  while (!TryReadFromBuffer(buffer, result)) {
+    if (++read_attempts == kMaxReadAttemptsCount)
+      return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
 namespace content {
 
 DeviceMotionEventPump::DeviceMotionEventPump(RenderThread* thread)
-    : DeviceSensorMojoClientMixin<
-          DeviceSensorEventPump<blink::WebDeviceMotionListener>,
-          device::mojom::MotionSensor>(thread) {}
+    : PlatformEventObserver<blink::WebDeviceMotionListener>(thread),
+      accelerometer_(this, device::mojom::SensorType::ACCELEROMETER),
+      linear_acceleration_sensor_(
+          this,
+          device::mojom::SensorType::LINEAR_ACCELERATION),
+      gyroscope_(this, device::mojom::SensorType::GYROSCOPE),
+      state_(PumpState::STOPPED) {}
 
 DeviceMotionEventPump::~DeviceMotionEventPump() {
+  PlatformEventObserver<blink::WebDeviceMotionListener>::StopIfObserving();
 }
 
-void DeviceMotionEventPump::FireEvent() {
-  DCHECK(listener());
-  device::MotionData data;
-  if (reader_->GetLatestData(&data) && data.all_available_sensors_are_active)
-    listener()->DidChangeDeviceMotion(data);
+void DeviceMotionEventPump::Start(blink::WebPlatformEventListener* listener) {
+  DVLOG(2) << "requested start";
+
+  if (state_ != PumpState::STOPPED)
+    return;
+
+  DCHECK(!timer_.IsRunning());
+
+  state_ = PumpState::PENDING_START;
+  PlatformEventObserver<blink::WebDeviceMotionListener>::Start(listener);
 }
 
-bool DeviceMotionEventPump::InitializeReader(base::SharedMemoryHandle handle) {
-  if (!reader_)
-    reader_.reset(new DeviceMotionSharedMemoryReader());
-  return reader_->Initialize(handle);
+void DeviceMotionEventPump::Stop() {
+  DVLOG(2) << "requested stop";
+
+  if (state_ == PumpState::STOPPED)
+    return;
+
+  DCHECK((state_ == PumpState::PENDING_START && !timer_.IsRunning()) ||
+         (state_ == PumpState::RUNNING && timer_.IsRunning()));
+
+  if (timer_.IsRunning())
+    timer_.Stop();
+
+  PlatformEventObserver<blink::WebDeviceMotionListener>::Stop();
+  state_ = PumpState::STOPPED;
+}
+
+void DeviceMotionEventPump::SendStartMessage() {
+  auto request = mojo::MakeRequest(&sensor_provider_);
+
+  // When running layout tests, those observers should not listen to the
+  // actual hardware changes. In order to make that happen, don't connect
+  // the other end of the mojo pipe to anything.
+  if (RenderThreadImpl::current() &&
+      !RenderThreadImpl::current()->layout_test_mode()) {

[timvolodine, 2017/06/07 20:37:35]

nit: maybe just do an early return in case it's in test mode, would be easier to
read I think.

[juncai, 2017/06/07 23:05:07]

Done.

+    if (!accelerometer_.sensor && !linear_acceleration_sensor_.sensor &&
+        !gyroscope_.sensor) {
+      RenderThread::Get()->GetConnector()->BindInterface(
+          device::mojom::kServiceName, std::move(request));
+      sensor_provider_.set_connection_error_handler(
+          base::Bind(&DeviceMotionEventPump::HandleSensorProviderError,
+                     base::Unretained(this)));
+      GetSensor(&accelerometer_);
+      GetSensor(&linear_acceleration_sensor_);
+      GetSensor(&gyroscope_);
+    } else {
+      if (accelerometer_.sensor)
+        accelerometer_.sensor->Resume();
+
+      if (linear_acceleration_sensor_.sensor)
+        linear_acceleration_sensor_.sensor->Resume();
+
+      if (gyroscope_.sensor)
+        gyroscope_.sensor->Resume();
+
+      DidStart();
+    }
+  }
+}
+
+void DeviceMotionEventPump::SendStopMessage() {
+  if (accelerometer_.sensor)
+    accelerometer_.sensor->Suspend();

[timvolodine, 2017/06/07 20:37:35]

What happens when we close a window, will that stop the sensors by breaking the
pipe? Does this also mean that if I unregister all 'ondevicemotion' handlers the
sensors will be in suspended state? (does this result in some resources being
held?)

[juncai, 2017/06/07 23:05:07]

When a window is closed, the pipe is closed and the sensors are stopped. If all
device motion event listener are unregistered, the sensors will be in a
suspended state.

Related comments about using Resume() and Suspend() by reillyg@ is at:
https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic...

[timvolodine, 2017/06/08 20:26:51]

I see, so it's optimized for visibility change. Perhaps add a comment to explain
this (something similar to what Reilly said).

[juncai, 2017/06/08 23:29:43]

Done.

+
+  if (linear_acceleration_sensor_.sensor)
+    linear_acceleration_sensor_.sensor->Suspend();
+
+  if (gyroscope_.sensor)
+    gyroscope_.sensor->Suspend();
 }
 
 void DeviceMotionEventPump::SendFakeDataForTesting(void* fake_data) {
   device::MotionData data = *static_cast<device::MotionData*>(fake_data);
+  listener()->DidChangeDeviceMotion(data);
+}
+
+DeviceMotionEventPump::SensorEntry::SensorEntry(
+    DeviceMotionEventPump* pump,
+    device::mojom::SensorType sensor_type)
+    : event_pump(pump), type(sensor_type), client_binding(this) {}
+
+DeviceMotionEventPump::SensorEntry::~SensorEntry() {}
+
+void DeviceMotionEventPump::SensorEntry::RaiseError() {
+  HandleSensorError();
+}
+
+void DeviceMotionEventPump::SensorEntry::SensorReadingChanged() {
+  // Since DeviceMotionEventPump::FireEvent is called in a certain
+  // frequency, the |shared_buffer| is read frequently, so this
+  // method doesn't need to be implemented.

[timvolodine, 2017/06/07 20:37:36]

sorry if I asked this previously, can we put NOTREACHED() here? (since I assume
we read the buffer ourselves and don't need this signal)

[juncai, 2017/06/07 23:05:07]

I don't think NOREACHED() can be put here.

This question was asked and answered here:
https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device...

I copied the answer below:
I don't think a DCHECK() or NOTREACHED() can be used here, since this method is
called if a sensor is on ON_CHANGE mode and our code doesn't set this mode. The
sensor mode is a return value of the mojo call GetSensor():
https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/...

[timvolodine, 2017/06/08 20:26:51]

This seems strange - we would get unnecessary high frequency mojo IPC calls?
maybe we should set the mode to explicitly reflect the shared memory usage (I
thought it was supported in generic_sensors/)?

[juncai, 2017/06/08 23:29:43]

Yes, we would get high frequency mojo IPC calls. I am working on a CL to fix it
by adding a flag to the sensor configuration, so that we can set the flag to
notify the browser side not to call SensorReadingChanged() function even when
the sensor reporting mode is ON_CHANGE. So eventually NOREACHED() can be added
here.

[juncai, 2017/06/13 23:02:18]

Since the CL:
https://codereview.chromium.org/2927263002
was landed. NOTREACHED() can be added here.

Done.

+}
+
+void DeviceMotionEventPump::SensorEntry::OnSensorCreated(
+    device::mojom::SensorInitParamsPtr params,
+    device::mojom::SensorClientRequest client_request) {
+  if (!params) {
+    HandleSensorError();
+    if (event_pump->CanStart())
+      event_pump->DidStart();
+    return;
+  }
+
+  constexpr size_t kReadBufferSize = sizeof(device::SensorReadingSharedBuffer);
+
+  DCHECK_EQ(0u, params->buffer_offset % kReadBufferSize);
 
+  mode = params->mode;
+  default_config = params->default_configuration;
+
+  DCHECK(sensor.is_bound());
+  client_binding.Bind(std::move(client_request));
+
+  shared_buffer_handle = std::move(params->memory);
+  DCHECK(!shared_buffer);
+  shared_buffer =
+      shared_buffer_handle->MapAtOffset(kReadBufferSize, params->buffer_offset);
+
+  if (!shared_buffer) {
+    HandleSensorError();
+    if (event_pump->CanStart())
+      event_pump->DidStart();
+    return;
+  }
+
+  DCHECK_GT(params->minimum_frequency, 0.0);
+  DCHECK_GE(params->maximum_frequency, params->minimum_frequency);
+  constexpr double kMaxAllowedFrequency =
+      device::mojom::SensorConfiguration::kMaxAllowedFrequency;

[timvolodine, 2017/06/07 20:37:35]

nit: probably no need for constexpr?

[juncai, 2017/06/07 23:05:07]

Done.

[timvolodine, 2017/06/08 20:26:51]

nit:
I actually meant putting
device::mojom::SensorConfiguration::kMaxAllowedFrequency directly inside the
DCHECK_GE (if it's not used anywhere else)

[juncai, 2017/06/08 23:29:43]

I see.

Done.

+  DCHECK_GE(kMaxAllowedFrequency, params->maximum_frequency);
+
+  default_config.set_frequency(kDefaultPumpFrequencyHz);
+
+  sensor->AddConfiguration(default_config,
+                           base::Bind(&SensorEntry::OnSensorAddConfiguration,
+                                      base::Unretained(this)));
+}
+
+void DeviceMotionEventPump::SensorEntry::OnSensorAddConfiguration(
+    bool success) {
+  if (!success)
+    HandleSensorError();
+  if (event_pump->CanStart())
+    event_pump->DidStart();
+}
+
+void DeviceMotionEventPump::SensorEntry::HandleSensorError() {
+  sensor.reset();
+  shared_buffer_handle.reset();
+  shared_buffer.reset();
+  client_binding.Close();
+}
+
+bool DeviceMotionEventPump::SensorEntry::SensorReadingCouldBeRead() {
+  if (!sensor)
+    return false;
+
+  const device::SensorReadingSharedBuffer* buffer =
+      static_cast<const device::SensorReadingSharedBuffer*>(
+          shared_buffer.get());
+  if (!UpdateSensorReading(buffer, &reading)) {
+    HandleSensorError();
+    return false;
+  }
+
+  return true;
+}
+
+void DeviceMotionEventPump::FireEvent() {
+  device::MotionData data;
+  data.interval = kDefaultPumpDelayMicroseconds;
+
+  DCHECK(listener());
+
+  GetDataFromSharedMemory(&data);
   listener()->DidChangeDeviceMotion(data);
 }
 
+void DeviceMotionEventPump::DidStart() {
+  DVLOG(2) << "did start sensor event pump";
+
+  if (state_ != PumpState::PENDING_START)
+    return;
+
+  DCHECK(!timer_.IsRunning());
+
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromMicroseconds(kDefaultPumpDelayMicroseconds),
+               this, &DeviceMotionEventPump::FireEvent);
+  state_ = PumpState::RUNNING;
+}
+
+bool DeviceMotionEventPump::CanStart() const {
+  if (accelerometer_.sensor && !accelerometer_.shared_buffer)
+    return false;
+
+  if (linear_acceleration_sensor_.sensor &&
+      !linear_acceleration_sensor_.shared_buffer) {
+    return false;
+  }
+
+  if (gyroscope_.sensor && !gyroscope_.shared_buffer)
+    return false;
+
+  return true;
+}
+
+void DeviceMotionEventPump::GetDataFromSharedMemory(device::MotionData* data) {
+  if (accelerometer_.SensorReadingCouldBeRead()) {
+    data->acceleration_including_gravity_x =
+        accelerometer_.reading.values[0].value();
+    data->acceleration_including_gravity_y =
+        accelerometer_.reading.values[1].value();
+    data->acceleration_including_gravity_z =
+        accelerometer_.reading.values[2].value();
+    data->has_acceleration_including_gravity_x = true;
+    data->has_acceleration_including_gravity_y = true;
+    data->has_acceleration_including_gravity_z = true;
+  }
+
+  if (linear_acceleration_sensor_.SensorReadingCouldBeRead()) {
+    data->acceleration_x =
+        linear_acceleration_sensor_.reading.values[0].value();
+    data->acceleration_y =
+        linear_acceleration_sensor_.reading.values[1].value();
+    data->acceleration_z =
+        linear_acceleration_sensor_.reading.values[2].value();
+    data->has_acceleration_x = true;
+    data->has_acceleration_y = true;
+    data->has_acceleration_z = true;
+  }
+
+  if (gyroscope_.SensorReadingCouldBeRead()) {
+    data->rotation_rate_alpha = gyroscope_.reading.values[0].value();
+    data->rotation_rate_beta = gyroscope_.reading.values[1].value();
+    data->rotation_rate_gamma = gyroscope_.reading.values[2].value();
+    data->has_rotation_rate_alpha = true;
+    data->has_rotation_rate_beta = true;
+    data->has_rotation_rate_gamma = true;
+  }
+}
+
+void DeviceMotionEventPump::GetSensor(SensorEntry* sensor_entry) {
+  auto request = mojo::MakeRequest(&sensor_entry->sensor);
+  sensor_provider_->GetSensor(sensor_entry->type, std::move(request),
+                              base::Bind(&SensorEntry::OnSensorCreated,
+                                         base::Unretained(sensor_entry)));
+  sensor_entry->sensor.set_connection_error_handler(base::Bind(
+      &SensorEntry::HandleSensorError, base::Unretained(sensor_entry)));
+}
+
+void DeviceMotionEventPump::HandleSensorProviderError() {
+  sensor_provider_.reset();
+}
+
 }  // namespace content