Implement DeviceMotionEvent API

content/browser/device_sensors/sensor_manager_chromeos.cc

Index: content/browser/device_sensors/sensor_manager_chromeos.cc
diff --git a/content/browser/device_sensors/sensor_manager_chromeos.cc b/content/browser/device_sensors/sensor_manager_chromeos.cc
index 899002ee9e38bafed49f97cb4743596b3f363d4f..eeb597bb38449c5b7886bb4c19d61e8e137926ea 100644
--- a/content/browser/device_sensors/sensor_manager_chromeos.cc
+++ b/content/browser/device_sensors/sensor_manager_chromeos.cc
@@ -6,9 +6,9 @@
 
 #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 {
@@ -18,66 +18,125 @@ 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;
-
-    // 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();
-  }
+  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();
 
   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_);
-  if (!orientation_buffer_)
-    return;
-
+    scoped_refptr<const chromeos::AccelerometerUpdate> update) {
   chromeos::AccelerometerSource source;
-  if (update.has(chromeos::ACCELEROMETER_SOURCE_SCREEN)) {
+  if (update->has(chromeos::ACCELEROMETER_SOURCE_SCREEN))
     source = chromeos::ACCELEROMETER_SOURCE_SCREEN;
-  } else if (update.has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD)) {
+  else if (update->has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD))
     source = chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD;
-  } else {
+  else
     return;
-  }
 
-  double x = update.get(source).x;
-  double y = update.get(source).y;
-  double z = update.get(source).z;
+  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;
 
   // Create a unit vector for trigonometry
   // TODO(jonross): Stop reversing signs for vector components once
@@ -110,12 +169,4 @@ void SensorManagerChromeOS::OnAccelerometerUpdated(
   orientation_buffer_->seqlock.WriteEnd();
 }
 
-void SensorManagerChromeOS::StartObservingAccelerometer() {
-  chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
-}
-
-void SensorManagerChromeOS::StopObservingAccelerometer() {
-  chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
-}
-
 }  // namespace content