Device Orientation API on Chrome OS

content/browser/device_sensors/sensor_manager_chromeos.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 "content/browser/device_sensors/sensor_manager_chromeos.h"
+
+#include "base/logging.h"
+#include "chromeos/accelerometer/accelerometer_reader.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) {
+  chromeos::AccelerometerReader::GetInstance()->AddObserver(this);

[timvolodine, 2015/01/21 13:16:57]

it looks a bit strange you are adding an observer on creation and removing only
on destruction. it seems adding an observer on StartFetchingDeviceOrientation
and removing it on StopFetchingDeviceOrientation would be a more logical
approach? wdyt?

[jonross, 2015/01/21 16:02:11]

I had originally considered that. However when I implement Device Motion API
this class will be handling separate buffers for the different APIs, all from
the same accelerometer source. I thought that this would be cleaner than
tracking which buffers are in use at each start/stop.

Though I would not object to changing it.

[timvolodine, 2015/01/21 19:08:21]

I think the add/remove observer should generally correspond to the Start/Stop
semantics. It does not make sense to have an observer when nobody is listening.

Looking at your approach it seems that you could:
1. put the add/remove observer logic (and future logic) in sensor_manager
corresponding to start/stop methods, or
2. remove the start/stop methods from sensor_manager and perform add/remove
observer on construction/destruction as it is now. In this case the class would
probably be better called sensor_manager_observer and the logic for
creating/destroying would be handled by data_fetcher_shared_memory. (i.e. having
a live sensor_manager means we are listening)

The problem with 2 is that it can be tricky to implement properly when
sensor_manager is destroyed. I am not sure whether AccelerometerReader is
thread-safe in that case. This is assuming the Add/Remove observer is called on
the IO-thread while the accelerometer events can come in on a different thread.

+}
+
+SensorManagerChromeOS::~SensorManagerChromeOS() {
+  chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
+}
+
+bool SensorManagerChromeOS::IsFetching() {
+  // TODO(jonross): When DeviceMotion API is implemented update this to check
+  // each of the different buffers.
+  return orientation_buffer_ != nullptr;
+}
+
+bool SensorManagerChromeOS::StartFetchingDeviceOrientationData(
+    DeviceOrientationHardwareBuffer* buffer) {
+  DCHECK(buffer);
+  base::AutoLock autolock(orientation_buffer_lock_);
+  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();
+  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;
+  return true;
+}
+
+void SensorManagerChromeOS::OnAccelerometerUpdated(
+    const ui::AccelerometerUpdate& update) {
+  base::AutoLock autolock(orientation_buffer_lock_);
+  if (!orientation_buffer_)
+    return;
+
+  ui::AccelerometerSource source;
+  if (update.has(ui::ACCELEROMETER_SOURCE_SCREEN)) {
+    source = ui::ACCELEROMETER_SOURCE_SCREEN;
+  } else if (update.has(ui::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD)) {
+    source = ui::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)
+  // y = -cos(gamma) * sin(beta)
+  // z = cos(beta) * cos(gamma)
+  // With only accelerometer alpha cannot be provided.
+  double beta = kRad2deg * atan2(data.y(), data.z());
+  double gamma = kRad2deg * asin(data.x());
+
+  // Convert beta and gamma to fit the intervals in the specification. Beta is
+  // [-180, 180) and gamma is [-90, 90).
+  if (beta >= 180.0f)
+    beta = -180.0f;
+  if (gamma >= 90.0f)
+    gamma = -90.0f;
+  // TODO: move chromeos/accelerometer to /content/browser/device_sensors/ and

[timvolodine, 2015/01/21 13:16:57]

is this still relevant?

[jonross, 2015/01/21 16:02:10]

Nope, missed removing it.

+  // have it generate the beta and gamma values. crbug.com/431865
+  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();
+}
+
+}  // namespace content