DeviceOrientation API on ChromeOS

content/browser/device_sensors/sensor_manager_chromeos.h

+// 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.
+
+#ifndef CONTENT_BROWSER_DEVICE_SENSORS_SENSOR_MANAGER_CHROMEOS_H_
+#define CONTENT_BROWSER_DEVICE_SENSORS_SENSOR_MANAGER_CHROMEOS_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "content/common/content_export.h"
+#include "content/common/device_sensors/device_orientation_hardware_buffer.h"
+#include "content/public/browser/sensor_manager_delegate_chromeos.h"
+
+template <typename T>
+struct DefaultSingletonTraits;
+
+namespace content {
+
+class SensorManagerChromeOSTest;

[timvolodine, 2014/11/28 17:46:06]

no need for this?

[jonross, 2014/12/02 16:13:36]

Currently used for calling OnAccelerationIncludingGravity.

However I have no objections to making that method public.

[timvolodine, 2014/12/03 13:19:55]

I think you can drop this line, you already have a friend class declaration
below.

+class SensorManagerDelegateChromeOS;
+
+// Notifies Chrome OS sensors to start and stop observing accelerometer sensors.
+// Provides a callback, which interprets accelerometer data for
+// Device Orientation events.
+class CONTENT_EXPORT SensorManagerChromeOS
+    : public base::RefCountedThreadSafe<SensorManagerChromeOS> {

[timvolodine, 2014/11/26 19:32:19]

are you sure about this? the class is a singleton why does it need to be
ref-counted?

[jonross, 2014/11/26 23:17:56]

This is based on the example from callback.h for binding a class method to be
used across threads.
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=54

I did not test this without the RefCountedThreadSafe, so I am not sure if it
would be fine.

[timvolodine, 2014/11/28 17:46:06]

normally you can ensure the callback of a singleton will live long enough, i.e.
during shutdown StopFetchingAllDeviceData() is guaranteed to be called (in
device_inertial_sensor_service).
It seems you could drop reference counting and init the callback at construction
time to:
base::Bind(&SensorManagerChromeOS::OnAccelerationIncludingGravity,
base::Unretained(this));

+ public:
+  // Retrieves the singleton instance.
+  static SensorManagerChromeOS* GetInstance();

[timvolodine, 2014/11/26 19:32:19]

ideally we should get rid of this singleton as well. maybe add a todo to
consider that once crbug.com/431865 is done.

[jonross, 2014/12/02 16:13:36]

Done.

+
+  // Sets the delegate which observes accelerometer events.
+  void SetDelegate(SensorManagerDelegateChromeOS* delegate);
+
+  // Begins monitoring of orientation events, the shared memory of |buffer| will
+  // be updated upon subsequent events.
+  bool StartFetchingDeviceOrientationData(
+      DeviceOrientationHardwareBuffer* buffer);
+
+  // Stops monitoring orientation events.
+  void StopFetchingDeviceOrientationData();
+
+ protected:
+  virtual ~SensorManagerChromeOS();
+
+ private:
+  friend class base::RefCountedThreadSafe<SensorManagerChromeOS>;
+  friend class SensorManagerChromeOSTest;
+  friend struct DefaultSingletonTraits<SensorManagerChromeOS>;
+
+  // Shared memory to to update.
+  DeviceOrientationHardwareBuffer* orientation_buffer_;
+
+  // Synchronize orientation_buffer_ across threads.
+  base::Lock orientation_buffer_lock_;
+
+  // Thread safe tracking of callback for Chrome OS sensors.
+  scoped_refptr<SensorManagerChromeOS> callback_ref_;
+
+  // Platform delegate that receives actual accelerometer updates, and provides
+  // them via a callback.
+  SensorManagerDelegateChromeOS* delegate_;

[timvolodine, 2014/11/26 19:32:19]

I assume Shell owns (and destructs) this. it's probably fine, but to
double-check: can the delegate_ be used after it is destructed by Shell?

[jonross, 2014/11/26 23:17:56]

After the Shell destructs delegate_ it cannot be used.

However content::SensorManagerChromeOS is destructed before Shell destructs the
delegate_

[timvolodine, 2014/11/28 17:46:06]

ok, that's what I was wondering: the order of destruction.

generally speaking I think it would be good to expose something from chromeOS to
content instead of having this delegate. Something similar like we did for
battery api?:

e.g. device/battery/battery_status_manager_chromeos.cc
chromeos::PowerManagerClient* power_client =
    chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
power_client->AddObserver(this);

that way the content::SensorManagerChromeOS doesn't have to be a singleton. But
that's probably something you had in mind for the TODO already..

[jonross, 2014/12/02 16:13:36]

Extra documentation and a TODO added.

+
+  SensorManagerChromeOS();

[timvolodine, 2014/11/26 19:32:19]

this should come before the members

[jonross, 2014/12/02 16:13:36]

Done.

+
+  // Callback that receives accelerometer events. These events are interpreted
+  // into orientation events.
+  void OnAccelerationIncludingGravity(double x, double y, double z);

[timvolodine, 2014/11/26 19:32:19]

this also before the members

[jonross, 2014/12/02 16:13:36]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(SensorManagerChromeOS);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DEVICE_SENSORS_SENSOR_MANAGER_CHROMEOS_H_