Implement DeviceOrientation API on Windows

content/browser/device_orientation/data_fetcher_impl_win.cc

Index: content/browser/device_orientation/data_fetcher_impl_win.cc
diff --git a/content/browser/device_orientation/data_fetcher_impl_win.cc b/content/browser/device_orientation/data_fetcher_impl_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b34bebd8d57c39dbebf1f98f9be2dc20375fef65
--- /dev/null
+++ b/content/browser/device_orientation/data_fetcher_impl_win.cc
@@ -0,0 +1,211 @@
+// Copyright (c) 2013 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_orientation/data_fetcher_impl_win.h"
+
+#include <InitGuid.h>
+#include <PortableDeviceTypes.h>
+#include <Sensors.h>
+
+#include "base/logging.h"
+#include "content/browser/device_orientation/orientation.h"
+
+using base::win::ScopedComPtr;

[sail, 2013/04/09 14:43:58]

I think you should avoid this.

[nhu, 2013/04/09 18:13:16]

Done.

+
+namespace {
+
+// This should match ProviderImpl::kDesiredSamplingIntervalMs.
+const int kPeriodInMilliseconds = 100;

[Peter Beverloo, 2013/04/09 15:34:15]

We're trying to move to updating Device Orientation (and Motion) data at
50-60Hz.  Do you expect this, performance wise, to be an issue for the Windows
implementation?

[nhu, 2013/04/09 18:13:16]

I have no device at hand now. I could do some tests on my devices on 50-60Hz
update rate. I will get back to you maybe tomorrow.

[nhu, 2013/04/10 16:51:01]

I did a brief performance evaluation today. The data is collected on a i3 Lenovo
Yoga ultrabook. The test site is 
http://www.html5rocks.com/en/tutorials/device/orientation/deviceorientationsa....
The data is:

app          |  CPU usage | C0 residency |
native app   |   1%       |   7%         |
chrome (10Hz)|   5%       |   30%        |
chrome (50Hz)|   10%      |   71%        |

The native app is driven by Windows sensor event other than polling.

I have to say the polling thread in current chrome implementation will be a
power concern. Do you plan to change it to event driven other than polling when
platform supports event notification (Windows and Android)?

[Peter Beverloo, 2013/04/12 15:52:10]

That's very, very useful, thank you!  Tim Volodine (timvolodine@chromium.org) is
working on improving the Device Orientation implementation to work with events
as well as polling, which is necessary for Android, so this will be possible in
the future.

+
+}  // namespace
+
+namespace content {
+
+class DataFetcherImplWin::SensorEventSink : public ISensorEvents {

[sail, 2013/04/09 14:43:58]

inherit from base::win::IUnknownImpl

[nhu, 2013/04/09 18:13:16]

Done.

+ public:
+  explicit SensorEventSink(DataFetcherImplWin* const fetcher)
+      : fetcher_(fetcher),
+        ref_count_(0) {}
+
+  virtual ~SensorEventSink() {}
+
+  // IUnknown interface
+  STDMETHODIMP_(ULONG) AddRef() {

[sail, 2013/04/09 14:43:58]

virtual STDMETHODIMP AddRef() OVERRIDE
Same below

[nhu, 2013/04/09 18:13:16]

Done.

+    return InterlockedIncrement(&ref_count_);
+  }
+
+  STDMETHODIMP_(ULONG) Release() {
+    ULONG count = InterlockedDecrement(&ref_count_);
+    if (count == 0) {
+        delete this;
+        return 0;
+    }
+    return count;
+  }
+
+  STDMETHODIMP QueryInterface(REFIID requested_interface_id,
+                              void** return_value) {
+      if (return_value == NULL)
+        return E_POINTER;
+      if (requested_interface_id == __uuidof(IUnknown)) {
+          *return_value = static_cast<IUnknown*>(this);
+      } else if (requested_interface_id == __uuidof(ISensorEvents)) {

[sail, 2013/04/09 14:43:58]

only handle this and call super class for others

[nhu, 2013/04/09 18:13:16]

Done.

+          *return_value = static_cast<ISensorEvents*>(this);
+      } else {
+          *return_value = NULL;
+          return E_NOINTERFACE;
+      }
+      AddRef();
+      return S_OK;
+  }
+
+  // ISensorEvents interface
+  STDMETHODIMP OnEvent(ISensor *sensor,
+                       REFGUID event_id,
+                       IPortableDeviceValues *event_data) {
+    return S_OK;
+  }
+
+  STDMETHODIMP OnDataUpdated(ISensor *sensor, ISensorDataReport *new_data) {
+    if (NULL == new_data ||  NULL == sensor)
+      return E_INVALIDARG;
+
+    PROPVARIANT value = {};
+    bool can_provide_alpha = false;
+    bool can_provide_beta = false;
+    bool can_provide_gamma = false;
+    double alpha, beta, gamma;

[sail, 2013/04/09 14:59:14]

One line per variable declaration.
Actually it would be cleaner to just instantiate an Orientation object here and
pass it.

[nhu, 2013/04/09 18:13:16]

It's a good suggestion. Done.

+    if (SUCCEEDED(new_data->GetSensorValue(
+            SENSOR_DATA_TYPE_TILT_X_DEGREES, &value))) {
+      beta = value.fltVal;
+      can_provide_beta = true;
+    }
+    PropVariantClear(&value);
+    if (SUCCEEDED(new_data->GetSensorValue(
+            SENSOR_DATA_TYPE_TILT_Y_DEGREES, &value))) {
+      gamma = value.fltVal;
+      can_provide_gamma = true;
+    }
+    PropVariantClear(&value);
+    if (SUCCEEDED(new_data->GetSensorValue(
+            SENSOR_DATA_TYPE_TILT_Z_DEGREES, &value))) {
+      alpha = value.fltVal;
+      can_provide_alpha = true;
+      }

[Peter Beverloo, 2013/04/09 15:34:15]

nit: indentation.

[nhu, 2013/04/09 18:13:16]

Done.

+    PropVariantClear(&value);
+
+    fetcher_->OnOrientationData(can_provide_alpha, alpha,
+                                can_provide_beta, beta,
+                                can_provide_gamma, gamma);
+
+    return S_OK;
+  }
+
+  STDMETHODIMP OnLeave(REFSENSOR_ID sensor_id) {
+    return S_OK;
+  }
+
+  STDMETHODIMP OnStateChanged(ISensor* sensor, SensorState state) {
+    return S_OK;
+  }
+
+ private:
+  long ref_count_ ;
+  DataFetcherImplWin* const fetcher_;

[sail, 2013/04/09 14:43:58]

DISALLOW_COPY_... here

[nhu, 2013/04/09 18:13:16]

Done.

+};
+
+// Create a DataFetcherImplWin object and return NULL if no valid sensor found.
+DataFetcher* DataFetcherImplWin::Create() {

[sail, 2013/04/09 14:43:58]

"// static" above this line

[nhu, 2013/04/09 18:13:16]

Done.

+  scoped_ptr<DataFetcherImplWin> fetcher(new DataFetcherImplWin);
+  return fetcher->Init() ? fetcher.release() : NULL;
+}
+
+DataFetcherImplWin::~DataFetcherImplWin() {
+  if (sensor_.get())
+    sensor_->SetEventSink(NULL);
+}
+
+DataFetcherImplWin::DataFetcherImplWin() {
+}
+
+void DataFetcherImplWin::OnOrientationData(bool can_provide_alpha,

[sail, 2013/04/09 14:59:14]

Comment about what thread this may be called on.

[nhu, 2013/04/09 18:13:16]

Done.

+                                           double alpha,
+                                           bool can_provide_beta,
+                                           double beta,
+                                           bool can_provide_gamma,
+                                           double gamma) {
+  base::AutoLock autolock(last_orientation_lock_);
+  Orientation* orientation = new Orientation();
+  if (can_provide_alpha)
+    orientation->set_alpha(alpha);
+  if (can_provide_beta)
+    orientation->set_beta(beta);
+  if (can_provide_gamma)
+    orientation->set_gamma(gamma);
+  last_orientation_ = orientation;
+}
+
+const DeviceData* DataFetcherImplWin::GetDeviceData(DeviceData::Type type) {
+  if (type != DeviceData::kTypeOrientation)
+    return NULL;
+  return GetOrientation();
+}
+
+const Orientation* DataFetcherImplWin::GetOrientation() {
+  base::AutoLock autolock(last_orientation_lock_);
+  Orientation* orientation = new Orientation();

[sail, 2013/04/09 14:59:14]

Instead of instantiating a new object every 100ms can you just return the same
object when nothing has changed. The android implementation does a simple
current/next swap trick:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...

[nhu, 2013/04/09 18:13:16]

I've adopt the Android's implementation.

+  if (last_orientation_.get()) {
+    if (last_orientation_->can_provide_alpha())
+      orientation->set_alpha(last_orientation_->alpha());
+    if (last_orientation_->can_provide_beta())
+      orientation->set_beta(last_orientation_->beta());
+    if (last_orientation_->can_provide_gamma())
+      orientation->set_gamma(last_orientation_->gamma());
+    orientation->set_absolute(true);
+  }
+  return orientation;
+}
+
+bool DataFetcherImplWin::Init() {

[sail, 2013/04/09 14:43:58]

No abbreviations 

[nhu, 2013/04/09 18:13:16]

Done.

+  ScopedComPtr<ISensorManager> sensor_manager;
+  if (FAILED(::CoCreateInstance(CLSID_SensorManager,

[sail, 2013/04/09 14:43:58]

I think this requires Windows7 and above. Can you add a OS version in
DataFetcherImplWin::Create

[sail, 2013/04/09 14:43:58]

Use sensor_manager.CreateInstance()
Same below.

[sail, 2013/04/09 14:59:56]

Ohh. Does using portabledeviceguids make this work on WinXP?

[nhu, 2013/04/09 18:13:16]

Done.

[nhu, 2013/04/09 18:13:16]

The Minimum supported version of sensor framework API is windows 7.

[nhu, 2013/04/09 18:13:16]

Done.

+                                NULL,
+                                CLSCTX_INPROC_SERVER,
+                                __uuidof(ISensorManager),
+                                sensor_manager.ReceiveVoid())))
+    return false;
+
+  ScopedComPtr<ISensorCollection> sensor_collection;
+  if (FAILED(sensor_manager->GetSensorsByType(
+          SENSOR_TYPE_INCLINOMETER_3D, sensor_collection.Receive())))
+    return false;
+
+  if (FAILED(sensor_collection->GetAt(0, sensor_.Receive())))

[sail, 2013/04/09 14:43:58]

Is it ok to do this without checking the count first?

[nhu, 2013/04/09 18:13:16]

I agree that it's better to add the count checking.

+    return false;
+
+  ScopedComPtr<IPortableDeviceValues> device_values;
+  if (SUCCEEDED(CoCreateInstance(CLSID_PortableDeviceValues,
+                                 NULL,
+                                 CLSCTX_INPROC_SERVER,
+                                 __uuidof(IPortableDeviceValues),
+                                 device_values.ReceiveVoid()))) {
+    if (SUCCEEDED(device_values->SetUnsignedIntegerValue(
+            SENSOR_PROPERTY_CURRENT_REPORT_INTERVAL, kPeriodInMilliseconds))) {
+      ScopedComPtr<IPortableDeviceValues> return_values;
+      sensor_->SetProperties(device_values.get(), return_values.Receive());
+    }
+  }
+
+  SensorEventSink* sensor_event_class = new SensorEventSink(this);

[sail, 2013/04/09 14:43:58]

scoped_refptr<SensorEventSink>
sensor_event_class -> sensor_event_impl? (this is an instance, not a class)

[nhu, 2013/04/09 18:13:16]

I am not sure that if scoped_refptr could use with IUnknown other than
base::RefCounted, although both of the two classes have AddRef and Release.
Anyway, I guess it is a Chromium practice, so I change to that.
Done.

+  ScopedComPtr<ISensorEvents> sensor_events;
+  if (FAILED(sensor_event_class->QueryInterface(
+          __uuidof(ISensorEvents), sensor_events.ReceiveVoid())))
+    return false;
+  if (FAILED(sensor_->SetEventSink(sensor_events.get())))

[sail, 2013/04/09 14:43:58]

don't need the .get() ?

[Peter Beverloo, 2013/04/09 15:34:15]

nit: maybe a newline for readability?

[nhu, 2013/04/09 18:13:16]

Done.

[nhu, 2013/04/09 18:13:16]

Done.

+    return false;
+
+  return sensor_.get() != NULL;

[Peter Beverloo, 2013/04/09 15:34:15]

sensor_ should be set on line 184 and is being used on lines 196 and 205. 
Shouldn't we be certain that there's a pointer backing it by now?

[nhu, 2013/04/09 18:13:16]

You are right. Done.

+}
+
+}  // namespace content