Implement DeviceMotionEvent API

content/browser/device_sensors/sensor_manager_chromeos.cc

 // Copyright 2015 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 "content/browser/device_sensors/sensor_manager_chromeos.h"
 
 #include <math.h>
 
-#include "base/logging.h"
 #include "chromeos/accelerometer/accelerometer_reader.h"
 #include "chromeos/accelerometer/accelerometer_types.h"
+#include "content/browser/device_sensors/inertial_sensor_consts.h"
 #include "ui/gfx/geometry/vector3d_f.h"
 
 namespace {
 // Conversion ratio from radians to degrees.
 const double kRad2deg = 180.0 / M_PI;
 }
 
 namespace content {
 
-SensorManagerChromeOS::SensorManagerChromeOS() : orientation_buffer_(nullptr) {
+SensorManagerChromeOS::SensorManagerChromeOS()
+    : motion_buffer_(nullptr), orientation_buffer_(nullptr) {
 }
 
 SensorManagerChromeOS::~SensorManagerChromeOS() {
 }
 
+bool SensorManagerChromeOS::StartFetchingDeviceMotionData(
+    DeviceMotionHardwareBuffer* buffer) {

[timvolodine, 2015/03/09 15:55:57]

Do StartFetchingDeviceMotionData and OnAccelerometerUpdated get executed on the
same thread? If not I think you'll need a lock for motion_buffer_ to make sure
it's not updated concurrently. My suspicion is that
StartFetchingDeviceMotionData is executed on IO thread while
OnAccelerometerUpdated on some other thread?

[jonross, 2015/03/09 16:35:26]

AccelerometerReader has been updated to use a threadsafe observer. All
OnAccelerometerUpdated calls will be made on the same thread from which the
observer joined.

So for SensorManagerChromeOS joining is on IO thread, and OnAccelerometerUpdated
will be on IO thread.

[timvolodine, 2015/03/10 13:03:08]

I see, in that case how about adding a ThreadChecker to this class and
DCHECK(thread_checker_.CalledOnValidThread()) where appropriate?

[jonross, 2015/03/10 15:03:58]

Done.

+  DCHECK(buffer);
+  if (motion_buffer_)
+    return false;
+  motion_buffer_ = buffer;
+
+  motion_buffer_->seqlock.WriteBegin();
+  motion_buffer_->data.interval = std::max(

[timvolodine, 2015/03/09 15:55:57]

nit: maybe add a comment why this should be max(..)

[jonross, 2015/03/09 20:46:24]

Done.

+      kInertialSensorIntervalMicroseconds / 1000,
+      chromeos::AccelerometerReader::kDelayBetweenReadsMs);
+  motion_buffer_->seqlock.WriteEnd();
+
+  StartObservingAccelerometer();
+  return true;
+}
+
+bool SensorManagerChromeOS::StopFetchingDeviceMotionData() {
+  if (!motion_buffer_)
+    return false;

[timvolodine, 2015/03/09 15:55:57]

nit: maybe extra empty line for readability.

[jonross, 2015/03/09 20:46:24]

Done.

+  // Make sure to indicate that the sensor data is no longer available.
+  motion_buffer_->seqlock.WriteBegin();
+  motion_buffer_->data.allAvailableSensorsAreActive = false;
+  motion_buffer_->seqlock.WriteEnd();
+
+  motion_buffer_ = nullptr;
+
+  StopObservingAccelerometer();
+  return true;
+}
+
 bool SensorManagerChromeOS::StartFetchingDeviceOrientationData(
     DeviceOrientationHardwareBuffer* buffer) {
   DCHECK(buffer);
-  {
-    base::AutoLock autolock(orientation_buffer_lock_);
-    if (orientation_buffer_)
-      return false;
-    orientation_buffer_ = buffer;
+  if (orientation_buffer_)
+    return false;
+  orientation_buffer_ = buffer;
 
-    // No compass information, so we cannot provide absolute orientation.
-    orientation_buffer_->seqlock.WriteBegin();
-    orientation_buffer_->data.absolute = false;
-    orientation_buffer_->data.hasAbsolute = true;
-    orientation_buffer_->seqlock.WriteEnd();
-  }
+  // No compass information, so we cannot provide absolute orientation.
+  orientation_buffer_->seqlock.WriteBegin();
+  orientation_buffer_->data.absolute = false;
+  orientation_buffer_->data.hasAbsolute = true;
+  orientation_buffer_->seqlock.WriteEnd();
 
   StartObservingAccelerometer();
   return true;
 }
 
 bool SensorManagerChromeOS::StopFetchingDeviceOrientationData() {
-  {
-    base::AutoLock autolock(orientation_buffer_lock_);
-    if (!orientation_buffer_)
-      return false;
-    // Make sure to indicate that the sensor data is no longer available.
-    orientation_buffer_->seqlock.WriteBegin();
-    orientation_buffer_->data.allAvailableSensorsAreActive = false;
-    orientation_buffer_->seqlock.WriteEnd();
-    orientation_buffer_ = nullptr;
-  }
+  if (!orientation_buffer_)
+    return false;
+  // Make sure to indicate that the sensor data is no longer available.
+  orientation_buffer_->seqlock.WriteBegin();
+  orientation_buffer_->data.allAvailableSensorsAreActive = false;
+  orientation_buffer_->seqlock.WriteEnd();
+  orientation_buffer_ = nullptr;
 
   StopObservingAccelerometer();
   return true;
 }
 
 void SensorManagerChromeOS::OnAccelerometerUpdated(
-    const chromeos::AccelerometerUpdate& update) {
-  base::AutoLock autolock(orientation_buffer_lock_);
+    scoped_refptr<const chromeos::AccelerometerUpdate> update) {
+  chromeos::AccelerometerSource source;
+  if (update->has(chromeos::ACCELEROMETER_SOURCE_SCREEN))
+    source = chromeos::ACCELEROMETER_SOURCE_SCREEN;
+  else if (update->has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD))
+    source = chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD;
+  else
+    return;
+
+  double x = update->get(source).x;
+  double y = update->get(source).y;
+  double z = update->get(source).z;
+
+  GenerateMotionEvent(x, y, z);
+  GenerateOrientationEvent(x, y, z);
+}
+
+void SensorManagerChromeOS::StartObservingAccelerometer() {
+  if (chromeos::AccelerometerReader::GetInstance()->HasObserver(this))
+    return;
+  chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
+}
+
+void SensorManagerChromeOS::StopObservingAccelerometer() {
+  if (orientation_buffer_ || motion_buffer_)
+    return;

[timvolodine, 2015/03/09 15:55:57]

it's a bit strange to have this kind of logic, i.e. if motion_buffer_ is not
null I cannot stop observing accelerometer.. I don't think we use this approach
anywhere else, would a simple removeObserver be sufficient here?

[jonross, 2015/03/09 16:35:26]

It is not sufficient if we have two pages currently listening to different
events.

One page starts Device Motion, another Device Orientation.

If one page closes and stops listening to one event type, I don't want this to
also shutdown updates to the other.

Since both events are based on observing the same accelerometer updates I added
this check to only stop listening when neither event is active.

[timvolodine, 2015/03/10 13:03:08]

ok I see, IMHO it would be more readable to have something like
if (!orientation_buffer_)
  StopObservingAccelerometer();

inside the StopFetchingDeviceMotionData() method. Otherwise it looks like
StopObservingAccelerometer does not always actually stop observing..
(alternatively renaming this method to something like
"StopObservingAccelerometerIfPossible" would also be more clear).

[jonross, 2015/03/10 15:03:58]

Done.

Went with the check in each Stop*Buffer method.

+  DCHECK(chromeos::AccelerometerReader::GetInstance()->HasObserver(this));
+  chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
+}
+
+void SensorManagerChromeOS::GenerateMotionEvent(double x, double y, double z) {
+  if (!motion_buffer_)
+    return;
+
+  motion_buffer_->seqlock.WriteBegin();
+  motion_buffer_->data.accelerationIncludingGravityX = x;
+  motion_buffer_->data.hasAccelerationIncludingGravityX = true;
+  motion_buffer_->data.accelerationIncludingGravityY = y;
+  motion_buffer_->data.hasAccelerationIncludingGravityY = true;
+  motion_buffer_->data.accelerationIncludingGravityZ = z;
+  motion_buffer_->data.hasAccelerationIncludingGravityZ = true;
+  motion_buffer_->data.allAvailableSensorsAreActive = true;
+  motion_buffer_->seqlock.WriteEnd();
+}
+
+void SensorManagerChromeOS::GenerateOrientationEvent(double x,
+                                                     double y,
+                                                     double z) {
   if (!orientation_buffer_)
     return;
 
-  chromeos::AccelerometerSource source;
-  if (update.has(chromeos::ACCELEROMETER_SOURCE_SCREEN)) {
-    source = chromeos::ACCELEROMETER_SOURCE_SCREEN;
-  } else if (update.has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD)) {
-    source = chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD;
-  } else {
-    return;
-  }
-
-  double x = update.get(source).x;
-  double y = update.get(source).y;
-  double z = update.get(source).z;
-
   // Create a unit vector for trigonometry
   // TODO(jonross): Stop reversing signs for vector components once
   // accelerometer values have been fixed. crbug.com/431391
   // Ternaries are to remove -0.0f which gives incorrect trigonometrical
   // results.
   gfx::Vector3dF data(x, y ? -y : 0.0f, z ? -z : 0.0f);
   data.Scale(1.0f / data.Length());
 
   // Transform accelerometer to W3C angles, using the Z-X-Y Eulerangles matrix.
   // x = sin(gamma)
@@ skipping 11 matching lines @@
     gamma = -90.0f;
   orientation_buffer_->seqlock.WriteBegin();
   orientation_buffer_->data.beta = beta;
   orientation_buffer_->data.hasBeta = true;
   orientation_buffer_->data.gamma = gamma;
   orientation_buffer_->data.hasGamma = true;
   orientation_buffer_->data.allAvailableSensorsAreActive = true;
   orientation_buffer_->seqlock.WriteEnd();
 }
 
-void SensorManagerChromeOS::StartObservingAccelerometer() {
-  chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
-}
-
-void SensorManagerChromeOS::StopObservingAccelerometer() {
-  chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
-}
-
 }  // namespace content