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..260393fe0792c7c634a28359532677349dacb31b
--- /dev/null
+++ b/content/browser/device_orientation/data_fetcher_impl_win.cc
@@ -0,0 +1,208 @@
+// 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;
+
+namespace {
+
+// This should match ProviderImpl::kDesiredSamplingIntervalMs.
+const int kPeriodInMilliseconds = 100;
+
+}  // namespace
+
+namespace content {
+
+class DataFetcherImplWin::SensorEventSink : public ISensorEvents {
+ public:
+  SensorEventSink(DataFetcherImplWin* fetcher)

[bulach, 2013/04/02 15:48:07]

nit: use "explicit".

[nhu, 2013/04/03 03:06:52]

Done.

+      : fetcher_(fetcher),
+        ref_count_(0) {}
+
+  virtual ~SensorEventSink() {}
+
+  // IUnknown interface
+  STDMETHODIMP_(ULONG) AddRef() {
+    return InterlockedIncrement(&ref_count_);
+  }

[bulach, 2013/04/02 15:48:07]

nit: add a \n here, 44 and 67 (i.e., a \n in between the methods..)

[nhu, 2013/04/03 03:06:52]

Done.

+  STDMETHODIMP_(ULONG) Release() {
+    ULONG count = InterlockedDecrement(&ref_count_);
+    if (count == 0) {
+        delete this;
+        return 0;
+    }
+    return count;
+  }
+  STDMETHODIMP QueryInterface(REFIID iid, void** ppv)
+  {

[bulach, 2013/04/02 15:48:07]

nit: move { up to the end of 45..

also, please make the param names more chromy and less windowsy :) something
like:
REFIID requested_interface_id, void** return_value

[nhu, 2013/04/03 03:06:52]

Done.

+      if (ppv == NULL)
+        return E_POINTER;
+      if (iid == __uuidof(IUnknown)) {
+          *ppv = static_cast<IUnknown*>(this);
+      } else if (iid == __uuidof(ISensorEvents)) {
+          *ppv = static_cast<ISensorEvents*>(this);
+      } else {
+          *ppv = NULL;
+          return E_NOINTERFACE;
+      }
+      AddRef();
+      return S_OK;
+  }
+
+  // ISensorEvents interface
+  STDMETHODIMP OnEvent(ISensor *pSensor,

[bulach, 2013/04/02 15:48:07]

nit: * goes in the param type. also, as above, naming:
ISensor* sensor,
REFGUID event_id,
IPortableDeviceValues* event_data

[nhu, 2013/04/03 03:06:52]

Done.

+                       REFGUID eventID,
+                       IPortableDeviceValues *pEventData) {
+    return S_OK;
+  }
+  STDMETHODIMP OnDataUpdated(ISensor *pSensor, ISensorDataReport *pNewData) {

[bulach, 2013/04/02 15:48:07]

nit:
ISensor* sensor, ISensorDataReport* new_data

[nhu, 2013/04/03 03:06:52]

Done.

+    if (NULL == pNewData ||  NULL == pSensor)
+      return E_INVALIDARG;
+    PROPVARIANT pv = {};
+    bool can_provide_alpha, can_provide_beta, can_provide_gamma;
+    can_provide_alpha = can_provide_beta = can_provide_gamma = false;

[bulach, 2013/04/02 15:48:07]

nit: we normally do one per line, and then you can declare and assign each one
at the same time (i.e., three lines bool foo = false;)

[nhu, 2013/04/03 03:06:52]

Done.

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

[bulach, 2013/04/02 15:48:07]

nit: no need to test for fetcher_.. you could potentially mark the pointer as
const to indicate it has to be passed in the ctor and won't change.

[nhu, 2013/04/03 03:06:52]

Done.

+      fetcher_->OnOrientationData(can_provide_alpha, alpha,
+                                  can_provide_beta, beta,
+                                  can_provide_gamma, gamma);
+    }
+
+    return S_OK;
+  }
+  STDMETHODIMP OnLeave(REFSENSOR_ID sensorID) {
+    return S_OK;
+  }
+  STDMETHODIMP OnStateChanged(ISensor* pSensor, SensorState state) {
+    return S_OK;
+  }
+
+ private:
+  long ref_count_ ;
+  DataFetcherImplWin* fetcher_;
+};
+
+// Create a DataFetcherImplWin object and return NULL if no valid sensor found.
+DataFetcher* DataFetcherImplWin::Create() {
+  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,
+                                           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_);
+  scoped_refptr<Orientation> orientation(new Orientation());
+  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);
+  }
+  orientation->AddRef();

[bulach, 2013/04/02 15:48:07]

can we use the same mechanism as android and hold a current_orientation_ here?
it seems this AddRef will be unbalanced..

[nhu, 2013/04/03 03:06:52]

Thanks for your suggestion. Actually, I used to try the same mechanism as
Android implementation. But I found the issue of Android mechanism is the return
value will jump between two adjacent values, even when the sensor doesn't change
the report data. For example, on Chrome for Android, the beta (same as alpha and
gamma) will return 0 -> 1 -> 0 -> 1 forever when device is put on stable table.
I think the root cause is always swapping current_orientation and
next_orientation
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...).
I suppose it is a bug, your thoughts?
Thanks. It seems memory leak here. I am fixing it.

+  return orientation.get();
+}
+
+bool DataFetcherImplWin::Init() {
+  ScopedComPtr<ISensorManager> sensor_manager;
+  if (FAILED(::CoCreateInstance(CLSID_SensorManager,
+                                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())))

[bulach, 2013/04/02 15:48:07]

nit: continuation should be indented by 4 spaces

[nhu, 2013/04/03 03:06:52]

Done.

+    return false;
+
+  if (FAILED(sensor_collection->GetAt(0, sensor_.Receive())))
+    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,

[bulach, 2013/04/02 15:48:07]

nit: indent by 4

[nhu, 2013/04/03 03:06:52]

Done.

+                      kPeriodInMilliseconds))) {
+      ScopedComPtr<IPortableDeviceValues> return_values;
+      sensor_->SetProperties(device_values.get(), return_values.Receive());
+    }
+  }
+
+  SensorEventSink* sensor_event_class = new SensorEventSink(this);
+  ScopedComPtr<ISensorEvents> sensor_events;
+  if (FAILED(sensor_event_class->QueryInterface(
+                 __uuidof(ISensorEvents), sensor_events.ReceiveVoid())))

[bulach, 2013/04/02 15:48:07]

should sensor_event_class be deleted here and 202? also, nit: indent by 4

[nhu, 2013/04/03 03:06:52]

As my understanding and verification, the SensorEventSink object is referenced
(owned) by windows SensorApis thread after calling
SetEventSink(sensor_events.get()). When DataFetcherImplWin destructor calls
SetEventSink(NULL), the SensorEventSink object will be dereferenced and deleted.

+    return false;
+  if (FAILED(sensor_->SetEventSink(sensor_events.get())))
+    return false;
+
+  return sensor_.get() != NULL;
+}
+
+}  // namespace content
+

[bulach, 2013/04/02 15:48:07]

nit: remove extra \n