Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13433002:
Implement DeviceOrientation API on Windows (Closed)
|
Created:
7 years, 8 months ago by nhu Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
ssh://nhu@powerbuilder.sh.intel.com/home/www-data/git-repos/perc/chromium.git@sensor Visibility:
Public. |
DescriptionImplement DeviceOrientation API on Windows
The implemenation is based on Windows sensor framework COM API. It uses
the data of inclinometer 3D sensor which is a required fusion sensor
based on the accelerometer sensor.
BUG=224849
TEST=http://www.html5rocks.com/en/tutorials/device/orientation/deviceorientationsample.html
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195256
Patch Set 1 #
Total comments: 25
Patch Set 2 : Fixup according to bulach's comments (#3) #
Total comments: 46
Patch Set 3 : Update according to sail's and peter's comments #8, #9 and #11 #
Total comments: 21
Patch Set 4 : Update according to comment #14 and #15 #Patch Set 5 : Update according to comment #17 #
Messages
Total messages: 31 (0 generated)
Please kindly review. Thanks in advance.
thanks!! +peter and tim who are revamping this API and will be able to provide more in depth comments. I'll send my comments shortly.
some drive-by comments, mostly style nits. thanks again for implementing this! https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:27: SensorEventSink(DataFetcherImplWin* fetcher) nit: use "explicit". https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:36: } nit: add a \n here, 44 and 67 (i.e., a \n in between the methods..) https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:46: { 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 https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:62: STDMETHODIMP OnEvent(ISensor *pSensor, nit: * goes in the param type. also, as above, naming: ISensor* sensor, REFGUID event_id, IPortableDeviceValues* event_data https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:67: STDMETHODIMP OnDataUpdated(ISensor *pSensor, ISensorDataReport *pNewData) { nit: ISensor* sensor, ISensorDataReport* new_data https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:72: can_provide_alpha = can_provide_beta = can_provide_gamma = false; 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;) https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:92: if (fetcher_) { 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. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:161: orientation->AddRef(); can we use the same mechanism as android and hold a current_orientation_ here? it seems this AddRef will be unbalanced.. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:176: SENSOR_TYPE_INCLINOMETER_3D, sensor_collection.Receive()))) nit: continuation should be indented by 4 spaces https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:189: SENSOR_PROPERTY_CURRENT_REPORT_INTERVAL, nit: indent by 4 https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:199: __uuidof(ISensorEvents), sensor_events.ReceiveVoid()))) should sensor_event_class be deleted here and 202? also, nit: indent by 4 https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:208: nit: remove extra \n https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.h:25: // The Inlinometer 3D sensor (SENSOR_TYPE_INCLINOMETER_3D) is used to get the nit: typo, In"c"linometer
bulach, thanks for your review. Patch is updated and some comments are embedded inline. Please review again. Thanks. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:27: SensorEventSink(DataFetcherImplWin* fetcher) On 2013/04/02 15:48:07, bulach wrote: > nit: use "explicit". Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:36: } On 2013/04/02 15:48:07, bulach wrote: > nit: add a \n here, 44 and 67 (i.e., a \n in between the methods..) Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:46: { On 2013/04/02 15:48:07, bulach wrote: > 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 Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:62: STDMETHODIMP OnEvent(ISensor *pSensor, On 2013/04/02 15:48:07, bulach wrote: > nit: * goes in the param type. also, as above, naming: > ISensor* sensor, > REFGUID event_id, > IPortableDeviceValues* event_data Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:67: STDMETHODIMP OnDataUpdated(ISensor *pSensor, ISensorDataReport *pNewData) { On 2013/04/02 15:48:07, bulach wrote: > nit: > ISensor* sensor, ISensorDataReport* new_data Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:72: can_provide_alpha = can_provide_beta = can_provide_gamma = false; On 2013/04/02 15:48:07, bulach wrote: > 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;) Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:92: if (fetcher_) { On 2013/04/02 15:48:07, bulach wrote: > 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. Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:161: orientation->AddRef(); On 2013/04/02 15:48:07, bulach wrote: > can we use the same mechanism as android and hold a current_orientation_ here? 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? > it seems this AddRef will be unbalanced.. Thanks. It seems memory leak here. I am fixing it. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:176: SENSOR_TYPE_INCLINOMETER_3D, sensor_collection.Receive()))) On 2013/04/02 15:48:07, bulach wrote: > nit: continuation should be indented by 4 spaces Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:189: SENSOR_PROPERTY_CURRENT_REPORT_INTERVAL, On 2013/04/02 15:48:07, bulach wrote: > nit: indent by 4 Done. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.cc:199: __uuidof(ISensorEvents), sensor_events.ReceiveVoid()))) On 2013/04/02 15:48:07, bulach wrote: > should sensor_event_class be deleted here and 202? also, nit: indent by 4 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. https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/1/content/browser/device_orient... content/browser/device_orientation/data_fetcher_impl_win.h:25: // The Inlinometer 3D sensor (SENSOR_TYPE_INCLINOMETER_3D) is used to get the On 2013/04/02 15:48:07, bulach wrote: > nit: typo, In"c"linometer Done.
Hi folks, Do you have any comments on this? Thanks.
Hi Folks, If you have any comments, please let me know. Any of your thoughts are appreciated.
Hi, sail, This is my CL for bug https://code.google.com/p/chromium/issues/detail?id=224849. Could you have a look?
https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:14: using base::win::ScopedComPtr; I think you should avoid this. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:25: class DataFetcherImplWin::SensorEventSink : public ISensorEvents { inherit from base::win::IUnknownImpl https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:34: STDMETHODIMP_(ULONG) AddRef() { virtual STDMETHODIMP AddRef() OVERRIDE Same below https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:53: } else if (requested_interface_id == __uuidof(ISensorEvents)) { only handle this and call super class for others https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:115: DataFetcherImplWin* const fetcher_; DISALLOW_COPY_... here https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:119: DataFetcher* DataFetcherImplWin::Create() { "// static" above this line https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:170: bool DataFetcherImplWin::Init() { No abbreviations https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:172: if (FAILED(::CoCreateInstance(CLSID_SensorManager, I think this requires Windows7 and above. Can you add a OS version in DataFetcherImplWin::Create https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:172: if (FAILED(::CoCreateInstance(CLSID_SensorManager, Use sensor_manager.CreateInstance() Same below. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:184: if (FAILED(sensor_collection->GetAt(0, sensor_.Receive()))) Is it ok to do this without checking the count first? https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:200: SensorEventSink* sensor_event_class = new SensorEventSink(this); scoped_refptr<SensorEventSink> sensor_event_class -> sensor_event_impl? (this is an instance, not a class) https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:205: if (FAILED(sensor_->SetEventSink(sensor_events.get()))) don't need the .get() ?
https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:78: double alpha, beta, gamma; One line per variable declaration. Actually it would be cleaner to just instantiate an Orientation object here and pass it. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:132: void DataFetcherImplWin::OnOrientationData(bool can_provide_alpha, Comment about what thread this may be called on. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:157: Orientation* orientation = new Orientation(); 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... https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.h:30: // Factory function. We'll listen for events for the lifetime of this object. Shouldn't use "we" in comments.
https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:172: if (FAILED(::CoCreateInstance(CLSID_SensorManager, On 2013/04/09 14:43:58, sail wrote: > I think this requires Windows7 and above. Can you add a OS version in > DataFetcherImplWin::Create Ohh. Does using portabledeviceguids make this work on WinXP?
https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:19: const int kPeriodInMilliseconds = 100; 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? https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:95: } nit: indentation. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:205: if (FAILED(sensor_->SetEventSink(sensor_events.get()))) nit: maybe a newline for readability? https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:208: return sensor_.get() != NULL; 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? https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.h:8: #include <SensorsApi.h> Is SensorsApi.h available in anything pre-Windows 7?
https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.h:8: #include <SensorsApi.h> On 2013/04/09 15:34:15, Peter Beverloo wrote: > Is SensorsApi.h available in anything pre-Windows 7? We build against the Windows8 SDK. Linking is OK since this is all COM.
Patch set is updated. Please review. Thanks. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:14: using base::win::ScopedComPtr; On 2013/04/09 14:43:58, sail wrote: > I think you should avoid this. Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:19: const int kPeriodInMilliseconds = 100; On 2013/04/09 15:34:15, Peter Beverloo wrote: > 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? 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. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:25: class DataFetcherImplWin::SensorEventSink : public ISensorEvents { On 2013/04/09 14:43:58, sail wrote: > inherit from base::win::IUnknownImpl Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:34: STDMETHODIMP_(ULONG) AddRef() { On 2013/04/09 14:43:58, sail wrote: > virtual STDMETHODIMP AddRef() OVERRIDE > Same below Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:53: } else if (requested_interface_id == __uuidof(ISensorEvents)) { On 2013/04/09 14:43:58, sail wrote: > only handle this and call super class for others Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:78: double alpha, beta, gamma; On 2013/04/09 14:59:14, sail wrote: > One line per variable declaration. > Actually it would be cleaner to just instantiate an Orientation object here and > pass it. It's a good suggestion. Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:95: } On 2013/04/09 15:34:15, Peter Beverloo wrote: > nit: indentation. Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:115: DataFetcherImplWin* const fetcher_; On 2013/04/09 14:43:58, sail wrote: > DISALLOW_COPY_... here Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:119: DataFetcher* DataFetcherImplWin::Create() { On 2013/04/09 14:43:58, sail wrote: > "// static" above this line Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:132: void DataFetcherImplWin::OnOrientationData(bool can_provide_alpha, On 2013/04/09 14:59:14, sail wrote: > Comment about what thread this may be called on. Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:157: Orientation* orientation = new Orientation(); On 2013/04/09 14:59:14, sail wrote: > 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... I've adopt the Android's implementation. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:170: bool DataFetcherImplWin::Init() { On 2013/04/09 14:43:58, sail wrote: > No abbreviations Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:172: if (FAILED(::CoCreateInstance(CLSID_SensorManager, On 2013/04/09 14:43:58, sail wrote: > I think this requires Windows7 and above. Can you add a OS version in > DataFetcherImplWin::Create Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:172: if (FAILED(::CoCreateInstance(CLSID_SensorManager, On 2013/04/09 14:59:56, sail wrote: > On 2013/04/09 14:43:58, sail wrote: > > I think this requires Windows7 and above. Can you add a OS version in > > DataFetcherImplWin::Create > The Minimum supported version of sensor framework API is windows 7. > Ohh. Does using portabledeviceguids make this work on WinXP? https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:172: if (FAILED(::CoCreateInstance(CLSID_SensorManager, On 2013/04/09 14:43:58, sail wrote: > Use sensor_manager.CreateInstance() > Same below. Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:184: if (FAILED(sensor_collection->GetAt(0, sensor_.Receive()))) On 2013/04/09 14:43:58, sail wrote: > Is it ok to do this without checking the count first? I agree that it's better to add the count checking. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:200: SensorEventSink* sensor_event_class = new SensorEventSink(this); On 2013/04/09 14:43:58, sail wrote: > scoped_refptr<SensorEventSink> 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. > sensor_event_class -> sensor_event_impl? (this is an instance, not a class) Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:205: if (FAILED(sensor_->SetEventSink(sensor_events.get()))) On 2013/04/09 14:43:58, sail wrote: > don't need the .get() ? Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:205: if (FAILED(sensor_->SetEventSink(sensor_events.get()))) On 2013/04/09 15:34:15, Peter Beverloo wrote: > nit: maybe a newline for readability? Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:208: return sensor_.get() != NULL; On 2013/04/09 15:34:15, Peter Beverloo wrote: > 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? You are right. Done. https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.h:30: // Factory function. We'll listen for events for the lifetime of this object. On 2013/04/09 14:59:14, sail wrote: > Shouldn't use "we" in comments. Done.
LGTM! https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:64: Orientation* orientation = new Orientation(); scoped_refptr https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:116: if (sensor_.get()) I don't think you need the .get() here. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:180: scoped_refptr<SensorEventSink> sensor_event_impl = new SensorEventSink(this); use constructor instead of assign? (sensor_event_impl(new ..);) https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... content/content_browser.gypi:1113: '-lportabledeviceguids.lib', How big is this lib? Can we avoid using this and just weak link the CLSID? Specially now that you added the Win7 version check.
On 2013/04/05 03:04:26, nhu wrote: > Hi folks, > > Do you have any comments on this? Thanks. I pinged bulach. He's in London I think.
lgtm, many thanks! just some small nits, feel free to go ahead once peter is happy. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:52: STDMETHODIMP OnEvent(ISensor *sensor, nit: here and 54, 58 and 59, the style is: "type* name" rather than "type *name" https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:183: __uuidof(ISensorEvents), sensor_events.ReceiveVoid()); nit: too much indentation, should be +4 rather than +8 https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.h:37: virtual ~DataFetcherImplWin(); nit: destructor should be on top, before the static method https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.h:64: nit: extra \n
lgtm, thanks! Two minor nits, but please look at Marcus' and Sail's comments too. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:60: if (NULL == new_data || NULL == sensor) nit: two spaces after || https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:157: SENSOR_TYPE_INCLINOMETER_3D, sensor_collection.Receive()); nit: four space indent?
The patch set is updated according to comment #14, #16 and #17. Please review. Thanks. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:52: STDMETHODIMP OnEvent(ISensor *sensor, On 2013/04/10 07:59:46, bulach wrote: > nit: here and 54, 58 and 59, the style is: > "type* name" rather than > "type *name" Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:60: if (NULL == new_data || NULL == sensor) On 2013/04/10 08:59:51, Peter Beverloo wrote: > nit: two spaces after || Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:64: Orientation* orientation = new Orientation(); On 2013/04/09 20:53:53, sail wrote: > scoped_refptr Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:116: if (sensor_.get()) On 2013/04/09 20:53:53, sail wrote: > I don't think you need the .get() here. Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:157: SENSOR_TYPE_INCLINOMETER_3D, sensor_collection.Receive()); On 2013/04/10 08:59:51, Peter Beverloo wrote: > nit: four space indent? Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:180: scoped_refptr<SensorEventSink> sensor_event_impl = new SensorEventSink(this); On 2013/04/09 20:53:53, sail wrote: > use constructor instead of assign? (sensor_event_impl(new ..);) Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.cc:183: __uuidof(ISensorEvents), sensor_events.ReceiveVoid()); On 2013/04/10 07:59:46, bulach wrote: > nit: too much indentation, should be +4 rather than +8 Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... File content/browser/device_orientation/data_fetcher_impl_win.h (right): https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.h:37: virtual ~DataFetcherImplWin(); On 2013/04/10 07:59:46, bulach wrote: > nit: destructor should be on top, before the static method Done. https://codereview.chromium.org/13433002/diff/12002/content/browser/device_or... content/browser/device_orientation/data_fetcher_impl_win.h:64: On 2013/04/10 07:59:46, bulach wrote: > nit: extra \n Done. https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... content/content_browser.gypi:1113: '-lportabledeviceguids.lib', On 2013/04/09 20:53:53, sail wrote: > How big is this lib? Can we avoid using this and just weak link the CLSID? > Specially now that you added the Win7 version check. On my windows kits, it is 504KB. If we don't use it, the linker will fail with undefined reference CLSID error.
lgtm -- thank you again! Please wait for sail@ to reply on the portabledeviceguids.lib comment before landing, though. I launched a number of try-bots for you to make sure it compiles everywhere. So far, so good.
hi peter, some performance test data on polling interval. Please have a look. Your thoughts? https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:19: const int kPeriodInMilliseconds = 100; On 2013/04/09 15:34:15, Peter Beverloo wrote: > 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? 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)?
https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... content/content_browser.gypi:1113: '-lportabledeviceguids.lib', On 2013/04/10 09:46:42, nhu wrote: > On 2013/04/09 20:53:53, sail wrote: > > How big is this lib? Can we avoid using this and just weak link the CLSID? > > Specially now that you added the Win7 version check. > On my windows kits, it is 504KB. If we don't use it, the linker will fail with > undefined reference CLSID error. Unfortunately that's too big. I think you can fix the link error by adding the library that contains the sensor API to the DelayLoadDLLs section. Worst case we should just hardcode the CLSID.
On 2013/04/10 17:00:10, sail wrote: > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi > File content/content_browser.gypi (right): > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... > content/content_browser.gypi:1113: '-lportabledeviceguids.lib', > On 2013/04/10 09:46:42, nhu wrote: > > On 2013/04/09 20:53:53, sail wrote: > > > How big is this lib? Can we avoid using this and just weak link the CLSID? > > > Specially now that you added the Win7 version check. > > On my windows kits, it is 504KB. If we don't use it, the linker will fail with > > undefined reference CLSID error. > > Unfortunately that's too big. > > I think you can fix the link error by adding the library that contains the > sensor API to the DelayLoadDLLs section. > > Worst case we should just hardcode the CLSID. The linker should only link the piece of portabledeviceguids.lib which chrome needs. In my test, removing the portabledeviceguids.lib and sensor property report interval setting code (line 170 - 177 in data_fetcher_impl_win.cc), the final chrome.dll is 55,577,600 bytes. The chrome.dll with portabledeviceguids.lib is 55,578,624 bytes. It only increases 1024 bytes (1KB). It is not so big, right?
On 2013/04/10 16:51:01, nhu wrote: > hi peter, some performance test data on polling interval. Please have a look. > Your thoughts? > > https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... > File content/browser/device_orientation/data_fetcher_impl_win.cc (right): > > https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... > content/browser/device_orientation/data_fetcher_impl_win.cc:19: const int > kPeriodInMilliseconds = 100; > On 2013/04/09 15:34:15, Peter Beverloo wrote: > > 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? > > 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, any thoughts?
On 2013/04/11 01:48:01, nhu wrote: > On 2013/04/10 17:00:10, sail wrote: > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi > > File content/content_browser.gypi (right): > > > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... > > content/content_browser.gypi:1113: '-lportabledeviceguids.lib', > > On 2013/04/10 09:46:42, nhu wrote: > > > On 2013/04/09 20:53:53, sail wrote: > > > > How big is this lib? Can we avoid using this and just weak link the CLSID? > > > > Specially now that you added the Win7 version check. > > > On my windows kits, it is 504KB. If we don't use it, the linker will fail > with > > > undefined reference CLSID error. > > > > Unfortunately that's too big. > > > > I think you can fix the link error by adding the library that contains the > > sensor API to the DelayLoadDLLs section. > > > > Worst case we should just hardcode the CLSID. > > The linker should only link the piece of portabledeviceguids.lib which chrome > needs. In my test, removing the portabledeviceguids.lib and sensor property > report interval setting code (line 170 - 177 in data_fetcher_impl_win.cc), the > final chrome.dll is 55,577,600 bytes. > The chrome.dll with portabledeviceguids.lib is 55,578,624 bytes. It only > increases 1024 bytes (1KB). > > It is not so big, right? sail, any thoughts on this? thanks.
Peter, sail, I post comments to your questions, any of your thoughts are appreciated.
No further comments from my side. Thanks! https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... File content/browser/device_orientation/data_fetcher_impl_win.cc (right): https://codereview.chromium.org/13433002/diff/6001/content/browser/device_ori... content/browser/device_orientation/data_fetcher_impl_win.cc:19: const int kPeriodInMilliseconds = 100; On 2013/04/10 16:51:01, nhu wrote: > On 2013/04/09 15:34:15, Peter Beverloo wrote: > > 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? > > 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)? 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.
On 2013/04/11 01:48:01, nhu wrote: > On 2013/04/10 17:00:10, sail wrote: > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi > > File content/content_browser.gypi (right): > > > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... > > content/content_browser.gypi:1113: '-lportabledeviceguids.lib', > > On 2013/04/10 09:46:42, nhu wrote: > > > On 2013/04/09 20:53:53, sail wrote: > > > > How big is this lib? Can we avoid using this and just weak link the CLSID? > > > > Specially now that you added the Win7 version check. > > > On my windows kits, it is 504KB. If we don't use it, the linker will fail > with > > > undefined reference CLSID error. > > > > Unfortunately that's too big. > > > > I think you can fix the link error by adding the library that contains the > > sensor API to the DelayLoadDLLs section. > > > > Worst case we should just hardcode the CLSID. > > The linker should only link the piece of portabledeviceguids.lib which chrome > needs. In my test, removing the portabledeviceguids.lib and sensor property > report interval setting code (line 170 - 177 in data_fetcher_impl_win.cc), the > final chrome.dll is 55,577,600 bytes. > The chrome.dll with portabledeviceguids.lib is 55,578,624 bytes. It only > increases 1024 bytes (1KB). > > It is not so big, right? Hi sail, is this 1KB size increasing OK? do you have any suggestions?
On 2013/04/16 03:21:54, nhu wrote: > On 2013/04/11 01:48:01, nhu wrote: > > On 2013/04/10 17:00:10, sail wrote: > > > > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi > > > File content/content_browser.gypi (right): > > > > > > > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... > > > content/content_browser.gypi:1113: '-lportabledeviceguids.lib', > > > On 2013/04/10 09:46:42, nhu wrote: > > > > On 2013/04/09 20:53:53, sail wrote: > > > > > How big is this lib? Can we avoid using this and just weak link the > CLSID? > > > > > Specially now that you added the Win7 version check. > > > > On my windows kits, it is 504KB. If we don't use it, the linker will fail > > with > > > > undefined reference CLSID error. > > > > > > Unfortunately that's too big. > > > > > > I think you can fix the link error by adding the library that contains the > > > sensor API to the DelayLoadDLLs section. > > > > > > Worst case we should just hardcode the CLSID. > > > > The linker should only link the piece of portabledeviceguids.lib which chrome > > needs. In my test, removing the portabledeviceguids.lib and sensor property > > report interval setting code (line 170 - 177 in data_fetcher_impl_win.cc), the > > final chrome.dll is 55,577,600 bytes. > > The chrome.dll with portabledeviceguids.lib is 55,578,624 bytes. It only > > increases 1024 bytes (1KB). > > > > It is not so big, right? > > Hi sail, is this 1KB size increasing OK? do you have any suggestions? Hi, sorry for the slow turn around. It looks good to me. If the sherriffs have a problem with it we can revist the issue. LGTM!
On 2013/04/16 04:25:13, sail wrote: > On 2013/04/16 03:21:54, nhu wrote: > > On 2013/04/11 01:48:01, nhu wrote: > > > On 2013/04/10 17:00:10, sail wrote: > > > > > > > > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.gypi > > > > File content/content_browser.gypi (right): > > > > > > > > > > > > > > https://codereview.chromium.org/13433002/diff/12002/content/content_browser.g... > > > > content/content_browser.gypi:1113: '-lportabledeviceguids.lib', > > > > On 2013/04/10 09:46:42, nhu wrote: > > > > > On 2013/04/09 20:53:53, sail wrote: > > > > > > How big is this lib? Can we avoid using this and just weak link the > > CLSID? > > > > > > Specially now that you added the Win7 version check. > > > > > On my windows kits, it is 504KB. If we don't use it, the linker will > fail > > > with > > > > > undefined reference CLSID error. > > > > > > > > Unfortunately that's too big. > > > > > > > > I think you can fix the link error by adding the library that contains the > > > > sensor API to the DelayLoadDLLs section. > > > > > > > > Worst case we should just hardcode the CLSID. > > > > > > The linker should only link the piece of portabledeviceguids.lib which > chrome > > > needs. In my test, removing the portabledeviceguids.lib and sensor property > > > report interval setting code (line 170 - 177 in data_fetcher_impl_win.cc), > the > > > final chrome.dll is 55,577,600 bytes. > > > The chrome.dll with portabledeviceguids.lib is 55,578,624 bytes. It only > > > increases 1024 bytes (1KB). > > > > > > It is not so big, right? > > > > Hi sail, is this 1KB size increasing OK? do you have any suggestions? > > Hi, sorry for the slow turn around. It looks good to me. > If the sherriffs have a problem with it we can revist the issue. > > LGTM! Sail, thanks for your comments. Then I am going to trigger the commit.
Hi folks, It seems I cannot trigger the CQ. Could one of you guys help me to commit? Thanks.
Message was sent while issue was closed.
Committed patchset #5 manually as r195256. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
