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

content/renderer/device_sensors/device_motion_event_pump.cc

 // 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.
 
-#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,
+                       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),
+      pump_delay_microseconds_(kDefaultPumpDelayMicroseconds),

[Reilly Grant (use Gerrit), 2017/06/06 20:58:55]

This is a constant so it doesn't need to be a field.

[juncai, 2017/06/06 23:36:26]

Done.

+      state_(PumpState::STOPPED) {}
 
 DeviceMotionEventPump::~DeviceMotionEventPump() {
-}
-
-void DeviceMotionEventPump::FireEvent() {
-  DCHECK(listener());
-  device::MotionData data;
-  if (reader_->GetLatestData(&data) && data.all_available_sensors_are_active)
-    listener()->DidChangeDeviceMotion(data);
-}
-
-bool DeviceMotionEventPump::InitializeReader(base::SharedMemoryHandle handle) {
-  if (!reader_)
-    reader_.reset(new DeviceMotionSharedMemoryReader());
-  return reader_->Initialize(handle);
+  PlatformEventObserver<blink::WebDeviceMotionListener>::StopIfObserving();
+}
+
+void DeviceMotionEventPump::Start(blink::WebPlatformEventListener* listener) {
+  DVLOG(2) << "requested start";
+
+  if (state_ != PumpState::STOPPED)
+    return;
+
+  DCHECK(!timer_.IsRunning());
+
+  PlatformEventObserver<blink::WebDeviceMotionListener>::Start(listener);
+  state_ = PumpState::PENDING_START;
+}
+
+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()) {
+    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_);
+  }
+}
+
+void DeviceMotionEventPump::SendStopMessage() {
+  accelerometer_.HandleSensorError();
+  linear_acceleration_sensor_.HandleSensorError();
+  gyroscope_.HandleSensorError();

[Reilly Grant (use Gerrit), 2017/06/06 20:58:55]

I think this method should use Sensor::Suspend() instead of closing all the
pipes. This method gets called both when the page visibility changes and if the
last event listener is unregistered. My assumption is that removing the event
listener is more rare than the page visibility changing so we should optimize
for not doing extra work in this case.

[juncai, 2017/06/06 23:36:25]

Done.

 }
 
 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() {
+  client_binding.Close();

[Reilly Grant (use Gerrit), 2017/06/06 20:58:55]

This happens implicitly when the object is destroyed.

[juncai, 2017/06/06 23:36:25]

Done.

+}
+
+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.
+}
+
+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());

[Reilly Grant (use Gerrit), 2017/06/06 20:58:55]

I think you probably mean DCHECK(!client_binding.is_bound()), though that check
would be redundant since there is a similar one inside mojo::Binding::Bind.

[juncai, 2017/06/06 23:36:25]

It is similar to the following code:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/sensor...

+  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;
+  }
+
+  frequency_limits.first = params->minimum_frequency;
+  frequency_limits.second = params->maximum_frequency;
+
+  DCHECK_GT(frequency_limits.first, 0.0);
+  DCHECK_GE(frequency_limits.second, frequency_limits.first);
+  constexpr double kMaxAllowedFrequency =
+      device::mojom::SensorConfiguration::kMaxAllowedFrequency;
+  DCHECK_GE(kMaxAllowedFrequency, frequency_limits.second);

[Reilly Grant (use Gerrit), 2017/06/06 20:58:55]

frequency_limits doesn't seem to be used outside of these checks, which don't
seem necessary.

[juncai, 2017/06/06 23:36:25]

Done.

+
+  sensor->AddConfiguration(default_config,

[Reilly Grant (use Gerrit), 2017/06/06 20:58:55]

We want to ask for 60Hz here. Is it guaranteed that that is the default reported
by the browser process?

[juncai, 2017/06/06 23:36:25]

Done.

+                           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(pump_delay_microseconds_),
+               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