DeviceOrientation API on ChromeOS

content/public/browser/sensor_manager_delegate.h

+// Copyright 2014 The Chromium Authors. All rights reserved.

[timvolodine, 2014/11/24 14:51:05]

also should this file be called _chromeos, since it's chromeOS specific?

[jonross, 2014/11/24 22:53:01]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_BROWSER_SENSOR_MANAGER_DELEGATE_H_
+#define CONTENT_PUBLIC_BROWSER_SENSOR_MANAGER_DELEGATE_H_
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "content/common/content_export.h"
+
+template <typename T>
+struct DefaultSingletonTraits;
+
+namespace content {
+
+typedef base::Callback<void(double x, double y, double z)>
+    OrientationDataCallback;
+
+// For platforms with a push API for accelerometer sensors this delegate can be
+// implemented. Once notified to start fetching data, the callback provided can
+// be used to send the data.
+class CONTENT_EXPORT SensorManagerDelegate {

[timvolodine, 2014/11/24 14:51:05]

The implementation uses lots of singletons (e.g. both for
sensor_manager_chromeos and sensor_manager_delegate), can we get rid of them?
Here is an idea:
1. Have the sensor_manager_chromeos own a delegate as member (e.g. as
scoped_ptr)
2. In sensor_manager_delegate.h:

class SensorManagerDelegate {
 public:
  static scoped_ptr<SensorManagerDelegate> Create(const OrientationDataCallback&
callback);
  virtual StartFetchingDeviceOrientationData() = 0;
  virtual StopFetchingDeviceOrientationData() = 0;
};

sensor_manager_delegate_chromeos.cc would then implement a definition for
Create() using "new SensorManagerDelegateChromeOS".
to init its delegate the sensor_manager_chromeos would do:
sensor_manager_delegate_ = SensorManagerDelegate::Create(callback);

Does that make sense? We did a similar thing with Battery Status API (currently
in /device/battery/) I wonder if we can apply that pattern here as well.

[jonross, 2014/11/24 22:53:01]

This poses some problems with how the code is currently distributed between
content and ash.

I can get this to link, however it requires that ash implement and export the
static Create method. I don't like having a cross library linking dependency.

This also becomes trouble when the content_unittests tries to create a test
stub. That executable is not able to export the implementation of Create to the
content library.

Currently crbug.com/431865 tracks an issue where we intend to move the reading
of the hardware accelerometer to content/browser/device_sensors. With that
change this delegate can be removed, and a public observer can be created for
ash to listen for changes.

I've switched this class to be not a singleton, but to pass the delegate to
SensorManagerChromeOS. I'll add TODO to change this once the above issue is
resolved. 

+ public:
+  SensorManagerDelegate();
+  virtual ~SensorManagerDelegate();
+
+  // Returns the SensorManagerDelegate singleton
+  static SensorManagerDelegate* GetInstance();

[timvolodine, 2014/11/24 14:51:05]

you have both GetInstance() and public constructor methods which is somewhat
unsafe and unclear what should be used..

[jonross, 2014/11/24 22:53:01]

I've moved the constructor to protected. GetInstance has been removed with this
no longer being a singleton.

+
+  // True if a SensorManagerDelegate has been set.
+  static bool HasInstance();
+
+  // Allows platforms to provide a delegate.
+  static void SetDelegate(SensorManagerDelegate* delegate);
+
+  // Start processing accelerometer events for orientation. The values can be
+  // passed via |callback|
+  virtual void StartFetchingDeviceOrientationData(
+      const OrientationDataCallback& callback) = 0;
+
+  // Stop processing accelerometer events for orientation.
+  virtual void StopFetchingDeviceOrientationData() = 0;
+
+ private:
+  static SensorManagerDelegate* delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(SensorManagerDelegate);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_SENSOR_MANAGER_DELEGATE_H_