Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896583005:
Reland: Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors (Closed)
|
Created:
3 years, 7 months ago by juncai Modified:
3 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, shalamov, Peter Beverloo, riju_, blink-reviews, mlamouri+watch-blink_chromium.org, dglazkov+blink, darin-cc_chromium.org, wanming.lin, mlamouri+watch-sensors_chromium.org, haraken, einbinder+watch-test-runner_chromium.org, blink-reviews-api_chromium.org, jochen+watch_chromium.org, Mikhail Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors
//device/generic_sensors already has all the sensors that can be used
to implement the DeviceOrientation event:
https://w3c.github.io/deviceorientation/spec-source-orientation.html
Currently, //content/renderer/device_sensors uses sensors from
//device/sensors as its backend, and this is one of the CLs that refactor
//content/renderer/device_sensors to use sensors from
//device/generic_sensor as the backend and removes //device/sensors.
This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor.
The issue page contains the design doc link for this change.
BUG=721427
Review-Url: https://codereview.chromium.org/2896583005
Cr-Original-Commit-Position: refs/heads/master@{#480934}
Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab900596a77958fd
Review-Url: https://codereview.chromium.org/2896583005
Cr-Commit-Position: refs/heads/master@{#481791}
Committed: https://chromium.googlesource.com/chromium/src/+/33f72bfe8b9364e82990f92befa041a08d465d3c
Patch Set 1 : refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors #Patch Set 2 : updated test code #
Total comments: 22
Patch Set 3 : fix presubmit error #Patch Set 4 : address comments #
Total comments: 24
Patch Set 5 : updated device sensor browsertest #
Total comments: 38
Patch Set 6 : address comments #Patch Set 7 : removed WebDeviceMotionData.h #Patch Set 8 : continue using motion_data.h from //device/sensors #Patch Set 9 : clean up #include files #
Total comments: 14
Patch Set 10 : address more comments #
Total comments: 37
Patch Set 11 : address more comments #Patch Set 12 : added comments #Patch Set 13 : set suppress_on_change_events_ to true #Patch Set 14 : fix issues on Android, Mac and Windows #
Total comments: 2
Patch Set 15 : fix issues on Android, Mac, and Windows #Patch Set 16 : fix compile error on Windows #Messages
Total messages: 129 (79 generated)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor DeviceMotionEventPump to use device/generic_sensor instead of device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor BUG=721427 ========== to ========== Refactor DeviceMotionEventPump to use device/generic_sensor instead of device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. BUG=721427 ==========
Description was changed from ========== Refactor DeviceMotionEventPump to use device/generic_sensor instead of device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. BUG=721427 ========== to ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. BUG=721427 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
juncai@chromium.org changed reviewers: + reillyg@chromium.org
Please take a look.
https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (left): https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:318: IN_PROC_BROWSER_TEST_F(DeviceSensorBrowserTest, MotionTest) { Where did these tests move to? https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:30: device::mojom::kServiceName, std::move(request)); Move this logic to SendStartMessage so that GetSensor's assumption that sensor_provider_ is not null is correct (the error handler can reset it). https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:150: base::Bind(&SensorEntry::HandleSensorError, base::Unretained(this))); This should be set in GetSensor. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:230: // device motion spec does not require all sensors to be available. This comment and the code seem to be at odds. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:308: sensor->HandleSensorError(); I don't think we need to close all the sensors in response to this error. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:93: std::vector<std::unique_ptr<SensorEntry>> sensors_; Since this sensor type always uses exactly 3 sensors let's not use a vector for now. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump_unittest.cc (right): https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump_unittest.cc:85: sensors_[2]->type = device::mojom::SensorType::GYROSCOPE; Add a |type| parameter to the SensorEntry constructor. https://codereview.chromium.org/2896583005/diff/20001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/BUILD.gn (left): https://codereview.chromium.org/2896583005/diff/20001/device/generic_sensor/p... device/generic_sensor/public/interfaces/BUILD.gn:8: visibility = [ "//device/generic_sensor/public/cpp" ] The way device/generic_sensor/public/cpp/BUILD.gn is set up you should be able to depend on device/generic_sensor/public/cpp anywhere where you want to depend on device/generic_sensor/public/interfaces. https://codereview.chromium.org/2896583005/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h (right): https://codereview.chromium.org/2896583005/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h:84: "WebDeviceMotionData has wrong size"); This struct isn't used in shared memory so packing and this assert is unnecessary.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look. https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (left): https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:318: IN_PROC_BROWSER_TEST_F(DeviceSensorBrowserTest, MotionTest) { On 2017/05/22 20:29:46, Reilly Grant wrote: > Where did these tests move to? Talked to reillyg@ offline, these tests are already covered by device motion event pump unit tests and device motion event layout tests. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:30: device::mojom::kServiceName, std::move(request)); On 2017/05/22 20:29:46, Reilly Grant wrote: > Move this logic to SendStartMessage so that GetSensor's assumption that > sensor_provider_ is not null is correct (the error handler can reset it). Done. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:150: base::Bind(&SensorEntry::HandleSensorError, base::Unretained(this))); On 2017/05/22 20:29:46, Reilly Grant wrote: > This should be set in GetSensor. Done. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:230: // device motion spec does not require all sensors to be available. On 2017/05/22 20:29:46, Reilly Grant wrote: > This comment and the code seem to be at odds. Updated code to not have a state that a sensor is available but not active. So if a sensor completes the process of GetSensor() and AddConfiguration(), it is active. Otherwise it is not active. Done. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:308: sensor->HandleSensorError(); On 2017/05/22 20:29:46, Reilly Grant wrote: > I don't think we need to close all the sensors in response to this error. Done. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:93: std::vector<std::unique_ptr<SensorEntry>> sensors_; On 2017/05/22 20:29:46, Reilly Grant wrote: > Since this sensor type always uses exactly 3 sensors let's not use a vector for > now. Done. https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump_unittest.cc (right): https://codereview.chromium.org/2896583005/diff/20001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump_unittest.cc:85: sensors_[2]->type = device::mojom::SensorType::GYROSCOPE; On 2017/05/22 20:29:46, Reilly Grant wrote: > Add a |type| parameter to the SensorEntry constructor. Done. https://codereview.chromium.org/2896583005/diff/20001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/BUILD.gn (left): https://codereview.chromium.org/2896583005/diff/20001/device/generic_sensor/p... device/generic_sensor/public/interfaces/BUILD.gn:8: visibility = [ "//device/generic_sensor/public/cpp" ] On 2017/05/22 20:29:46, Reilly Grant wrote: > The way device/generic_sensor/public/cpp/BUILD.gn is set up you should be able > to depend on device/generic_sensor/public/cpp anywhere where you want to depend > on device/generic_sensor/public/interfaces. Done. https://codereview.chromium.org/2896583005/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h (right): https://codereview.chromium.org/2896583005/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h:84: "WebDeviceMotionData has wrong size"); On 2017/05/22 20:29:46, Reilly Grant wrote: > This struct isn't used in shared memory so packing and this assert is > unnecessary. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
timvolodine@chromium.org changed reviewers: + timvolodine@chromium.org
some drive-by comments, hope you don't mind ) https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (left): https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:318: IN_PROC_BROWSER_TEST_F(DeviceSensorBrowserTest, MotionTest) { On 2017/05/23 02:30:23, juncai wrote: > On 2017/05/22 20:29:46, Reilly Grant wrote: > > Where did these tests move to? > > Talked to reillyg@ offline, these tests are already covered by device motion > event pump unit tests and device motion event layout tests. what is the reason for removing this? strictly speaking the browser tests are end-to-end and are not the same as unit tests / layout tests.. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:28: class CONTENT_EXPORT DeviceMotionEventPump this implementation looks much more complicated now (and much larger as well). and the orientation side is not even covered in this patch. can this be simplified to have a simpler structure as at was before? https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... File content/test/data/device_sensors/device_sensors_null_test_with_alert.html (left): https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... content/test/data/device_sensors/device_sensors_null_test_with_alert.html:60: window.addEventListener('devicemotion', onMotion); why remove the motion test? https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h (left): https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h:40: class MotionData; I think this was introduced previously to get rid of the redundant WebDevice*Data classes in blink, not sure why we would bring them back?
https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (left): https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:318: IN_PROC_BROWSER_TEST_F(DeviceSensorBrowserTest, MotionTest) { On 2017/05/23 12:33:40, timvolodine wrote: > On 2017/05/23 02:30:23, juncai wrote: > > On 2017/05/22 20:29:46, Reilly Grant wrote: > > > Where did these tests move to? > > > > Talked to reillyg@ offline, these tests are already covered by device motion > > event pump unit tests and device motion event layout tests. > > what is the reason for removing this? strictly speaking the browser tests are > end-to-end and are not the same as unit tests / layout tests.. I think the layout tests at: //third_party/WebKit/LayoutTests/device_orientation/motion covers the browser tests here. For example, //third_party/WebKit/LayoutTests/device_orientation/motion/null-values.html is similar to the DeviceSensorBrowserTest's MotionNullTest. And the device motion event unit tests are updated to cover cases when different sensors are available and make sure the sensor data sent to the blink side is correct: https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:28: class CONTENT_EXPORT DeviceMotionEventPump On 2017/05/23 12:33:40, timvolodine wrote: > this implementation looks much more complicated now (and much larger as well). > and the orientation side is not even covered in this patch. can this be > simplified to have a simpler structure as at was before? This implementation contains some common code that the orientation side can use too, for example, the struct SensorEntry can be used by both. The original CL that does the change for both motion and orientation is: https://codereview.chromium.org/2885203004/ The above CL is closed in favor of several smaller CLs, and this CL is one of them. And I am going to have a follow-up CL for refactoring device orientation. And after all these refactoring is done, the code will be much simplified. https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... File content/test/data/device_sensors/device_sensors_null_test_with_alert.html (left): https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... content/test/data/device_sensors/device_sensors_null_test_with_alert.html:60: window.addEventListener('devicemotion', onMotion); On 2017/05/23 12:33:40, timvolodine wrote: > why remove the motion test? https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... explains the reason why removing the device motion browser test here. https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h (left): https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h:40: class MotionData; On 2017/05/23 12:33:40, timvolodine wrote: > I think this was introduced previously to get rid of the redundant > WebDevice*Data classes in blink, not sure why we would bring them back? Since the //device/sensors directory will be removed eventually, and the motion and orientation event will use //device/generic_sensors as their backend, so the device::MotionData is not used any more, and it can be removed in this CL. But we choose to not remove any code from //device/sensors until the refactoring of device motion and orientation is done, and this can make the purpose of each CL more clear.
Description was changed from ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. BUG=721427 ========== to ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 ==========
Thanks for the replies, some more comments below https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (left): https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:318: IN_PROC_BROWSER_TEST_F(DeviceSensorBrowserTest, MotionTest) { On 2017/05/23 21:33:55, juncai wrote: > On 2017/05/23 12:33:40, timvolodine wrote: > > On 2017/05/23 02:30:23, juncai wrote: > > > On 2017/05/22 20:29:46, Reilly Grant wrote: > > > > Where did these tests move to? > > > > > > Talked to reillyg@ offline, these tests are already covered by device motion > > > event pump unit tests and device motion event layout tests. > > > > what is the reason for removing this? strictly speaking the browser tests are > > end-to-end and are not the same as unit tests / layout tests.. > > I think the layout tests at: > //third_party/WebKit/LayoutTests/device_orientation/motion > covers the browser tests here. For example, > //third_party/WebKit/LayoutTests/device_orientation/motion/null-values.html > is similar to the DeviceSensorBrowserTest's MotionNullTest. > > And the device motion event unit tests are updated to cover cases when different > sensors are available and make sure the sensor data sent to the blink side is > correct: > https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... > The //third_party/WebKit/LayoutTests/device_orientation/motion uses testRunner, which sets the mock values and the test does not go beyond blink platform, i.e. it's not the same as the browsertests. Can we just keep the existing browser test here? or are there issues because the implementation switches over to a different 'back-end'? https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:108: // method doesn't need to be implemented. I guess this method shouldn't be called at all? https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:190: device::SensorReading* result) { maybe reader_ i.e. SharedMemorySeqlockReader can be used here? I think extracting the 'reading' functionality into a helper class would make it more readable/reusable.. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:243: if (accelerometer_sensor_.SensorReadingUpdated()) { nit: SensorReadingUpdated sounds a bit misleading, maybe something like SensorReadingCouldBeRead()? https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:28: class CONTENT_EXPORT DeviceMotionEventPump On 2017/05/23 21:33:55, juncai wrote: > On 2017/05/23 12:33:40, timvolodine wrote: > > this implementation looks much more complicated now (and much larger as well). > > and the orientation side is not even covered in this patch. can this be > > simplified to have a simpler structure as at was before? > > This implementation contains some common code that the orientation side can use > too, for example, the struct SensorEntry can be used by both. The original CL > that does the change for both motion and orientation is: > https://codereview.chromium.org/2885203004/ > The above CL is closed in favor of several smaller CLs, and this CL is one of > them. > > And I am going to have a follow-up CL for refactoring device orientation. And > after all these refactoring is done, the code will be much simplified. Thanks for the pointer, I'll have a look. Just not sure if it may be easier to provide some more support on the back-end side for this. There is quite some logic for the DeviceOrientation implementation in DeviceSensors.java (and its test) which would need to be accommodated somehow, are there plans for doing that? (and how?) https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:75: DeviceMotionEventPump* event_pump; we normally use _ for members, e.g. event_pump_? https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... File content/test/data/device_sensors/device_sensors_null_test_with_alert.html (left): https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... content/test/data/device_sensors/device_sensors_null_test_with_alert.html:60: window.addEventListener('devicemotion', onMotion); On 2017/05/23 21:33:55, juncai wrote: > On 2017/05/23 12:33:40, timvolodine wrote: > > why remove the motion test? > > https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... > explains the reason why removing the device motion browser test here. Removing this as such reduces test coverage. Can this be kept until the coverage is the same? https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h (left): https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h:40: class MotionData; On 2017/05/23 21:33:55, juncai wrote: > On 2017/05/23 12:33:40, timvolodine wrote: > > I think this was introduced previously to get rid of the redundant > > WebDevice*Data classes in blink, not sure why we would bring them back? > > Since the //device/sensors directory will be removed eventually, and the motion > and orientation event will use //device/generic_sensors as their backend, so the > device::MotionData is not used any more, and it can be removed in this CL. But > we choose to not remove any code from //device/sensors until the refactoring of > device motion and orientation is done, and this can make the purpose of each CL > more clear. The WebDevice*Data classes were explicitly removed in favor of moving the PODs to device/sensors in this CL https://codereview.chromium.org/2415083002. IIRC this was in context of the servicification project, and there is a more detailed explanation in description of that patch. So I am naturally concerned how this going back will have an impact on things? is it better to keep these POD definitions in the device namespace? (also the diff is wrong as it compares with DeviceMotionDispatcher)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot for the comments. Please take a look. https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (left): https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:318: IN_PROC_BROWSER_TEST_F(DeviceSensorBrowserTest, MotionTest) { On 2017/05/24 15:30:15, timvolodine wrote: > On 2017/05/23 21:33:55, juncai wrote: > > On 2017/05/23 12:33:40, timvolodine wrote: > > > On 2017/05/23 02:30:23, juncai wrote: > > > > On 2017/05/22 20:29:46, Reilly Grant wrote: > > > > > Where did these tests move to? > > > > > > > > Talked to reillyg@ offline, these tests are already covered by device > motion > > > > event pump unit tests and device motion event layout tests. > > > > > > what is the reason for removing this? strictly speaking the browser tests > are > > > end-to-end and are not the same as unit tests / layout tests.. > > > > I think the layout tests at: > > //third_party/WebKit/LayoutTests/device_orientation/motion > > covers the browser tests here. For example, > > //third_party/WebKit/LayoutTests/device_orientation/motion/null-values.html > > is similar to the DeviceSensorBrowserTest's MotionNullTest. > > > > And the device motion event unit tests are updated to cover cases when > different > > sensors are available and make sure the sensor data sent to the blink side is > > correct: > > > https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... > > > > The //third_party/WebKit/LayoutTests/device_orientation/motion uses testRunner, > which sets the mock values and the test does not go beyond blink platform, i.e. > it's not the same as the browsertests. > > Can we just keep the existing browser test here? or are there issues because the > implementation switches over to a different 'back-end'? Thanks for the comments. I updated device sensors browsertest. Done. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:108: // method doesn't need to be implemented. On 2017/05/24 15:30:16, timvolodine wrote: > I guess this method shouldn't be called at all? It depends on the sensor reporting mode: https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... It is called by the browser side to inform the renderer side that the sensor data is updated: https://cs.chromium.org/chromium/src/device/generic_sensor/sensor_impl.cc?l=5... But as the comments mentioned, since DeviceMotionEventPump::FireEvent() reads the shared memory in a certain frequency, this method doesn't need to to do anyting. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:190: device::SensorReading* result) { On 2017/05/24 15:30:16, timvolodine wrote: > maybe reader_ i.e. SharedMemorySeqlockReader can be used here? I think > extracting the 'reading' functionality into a helper class would make it more > readable/reusable.. The |shared_buffer| is a mojo::ScopedSharedBufferMapping https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.h?l=37 And the SharedMemorySeqlockReader needs to be initialized by a base::SharedMemoryHandle https://cs.chromium.org/chromium/src/content/renderer/shared_memory_seqlock_r... I don't think the SharedMemorySeqlockReader can be used here. I moved this function and UpdateSensorReading() out of the SensorEntry struct and make it more readable. Done. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:243: if (accelerometer_sensor_.SensorReadingUpdated()) { On 2017/05/24 15:30:16, timvolodine wrote: > nit: SensorReadingUpdated sounds a bit misleading, maybe something like > SensorReadingCouldBeRead()? Done. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:28: class CONTENT_EXPORT DeviceMotionEventPump On 2017/05/24 15:30:16, timvolodine wrote: > On 2017/05/23 21:33:55, juncai wrote: > > On 2017/05/23 12:33:40, timvolodine wrote: > > > this implementation looks much more complicated now (and much larger as > well). > > > and the orientation side is not even covered in this patch. can this be > > > simplified to have a simpler structure as at was before? > > > > This implementation contains some common code that the orientation side can > use > > too, for example, the struct SensorEntry can be used by both. The original CL > > that does the change for both motion and orientation is: > > https://codereview.chromium.org/2885203004/ > > The above CL is closed in favor of several smaller CLs, and this CL is one of > > them. > > > > And I am going to have a follow-up CL for refactoring device orientation. And > > after all these refactoring is done, the code will be much simplified. > > Thanks for the pointer, I'll have a look. Just not sure if it may be easier to > provide some more support on the back-end side for this. There is quite some > logic for the DeviceOrientation implementation in DeviceSensors.java (and its > test) which would need to be accommodated somehow, are there plans for doing > that? (and how?) Yes, there are plans to do that and the idea is similar to the closed CL: https://codereview.chromium.org/2885203004 From what I understand, the implementation logic in DeviceSensors.java is to use six kinds of sensor to gather motion/orientation data: https://cs.chromium.org/chromium/src/device/sensors/android/java/src/org/chro... And the //device/generic_sensor/ already provides all the sensors that are needed to implement the device motion/orientation event. Those are abstracted into the sensor.mojom interfaces. And for orientation there is a 3-way fallback approach which is implemented as: https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... And there are test code to cover the case when different sensors are available: https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:75: DeviceMotionEventPump* event_pump; On 2017/05/24 15:30:16, timvolodine wrote: > we normally use _ for members, e.g. event_pump_? This is inside a struct and it doesn't need the trailing underscore according to the Google C++ coding style: https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... File content/test/data/device_sensors/device_sensors_null_test_with_alert.html (left): https://codereview.chromium.org/2896583005/diff/60001/content/test/data/devic... content/test/data/device_sensors/device_sensors_null_test_with_alert.html:60: window.addEventListener('devicemotion', onMotion); On 2017/05/24 15:30:16, timvolodine wrote: > On 2017/05/23 21:33:55, juncai wrote: > > On 2017/05/23 12:33:40, timvolodine wrote: > > > why remove the motion test? > > > > > https://codereview.chromium.org/2896583005/diff/20001/content/browser/device_... > > explains the reason why removing the device motion browser test here. > > Removing this as such reduces test coverage. Can this be kept until the coverage > is the same? Updated the device sensors browsertest, and this file is not changed. Done. https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h (left): https://codereview.chromium.org/2896583005/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/device_orientation/WebDeviceMotionData.h:40: class MotionData; On 2017/05/24 15:30:16, timvolodine wrote: > On 2017/05/23 21:33:55, juncai wrote: > > On 2017/05/23 12:33:40, timvolodine wrote: > > > I think this was introduced previously to get rid of the redundant > > > WebDevice*Data classes in blink, not sure why we would bring them back? > > > > Since the //device/sensors directory will be removed eventually, and the > motion > > and orientation event will use //device/generic_sensors as their backend, so > the > > device::MotionData is not used any more, and it can be removed in this CL. But > > we choose to not remove any code from //device/sensors until the refactoring > of > > device motion and orientation is done, and this can make the purpose of each > CL > > more clear. > > The WebDevice*Data classes were explicitly removed in favor of moving the PODs > to device/sensors in this CL https://codereview.chromium.org/2415083002. IIRC > this was in context of the servicification project, and there is a more detailed > explanation in description of that patch. So I am naturally concerned how this > going back will have an impact on things? is it better to keep these POD > definitions in the device namespace? > > (also the diff is wrong as it compares with DeviceMotionDispatcher) These POD definitions will eventually be removed when the //content/renderer/device_sensors directory is removed and all its logic is implemented on the blink side. This is part of the onion-soup effort which I mentioned as the step 3 in the design doc. This onion-soup step will happen after //device/sensors directory is removed. The diff is confusing, but it is a new added file instead of a modified file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:156: class FakeAccelerometerSensor : public device::PlatformSensor { "meter" means "sensor", just call it a FakeAccelerometer. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:161: : PlatformSensor(type, std::move(mapping), provider) {} type -> device::mojom::SensorType::ACCELEROMETER https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:179: void StopSensor() override{}; nit: space before { https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:192: return default_configuration; return device::PlatformSensorConfiguration(60 /* frequency */); https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:204: : PlatformSensor(type, std::move(mapping), provider) {} type -> device::mojom::SensorType::LINEAR_ACCELERATION https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:235: return default_configuration; return device::PlatformSensorConfiguration(60 /* frequency */); https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:242: class FakeGyroscopeSensor : public device::PlatformSensor { "scope" also means "sensor", so just call it a FakeGyroscope. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:247: : PlatformSensor(type, std::move(mapping), provider) {} type -> device::mojom::SensorType::GYROSCOPE https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:278: return default_configuration; return device::PlatformSensorConfiguration(60 /* frequency */); https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:351: } scoped_refptr<device::PlatformSensor> sensor; switch (type) { case device::mojom::SensorType::ACCELEROMETER: if (accelerometer_is_available_) sensor = new FakeAccelerometer(std::move(mapping), this); break; case device::mojom::SensorType::LINEAR_ACCELERATION: if (linear_acceleration_sensor_is_available_) sensor = new FakeLinearAccelerationSensor(std::move(mapping), this); break; case device::mojom::SensorType::GYROSCOPE: if (gyroscope_is_available_) sensor = new FakeGyroscope(std::move(mapping), this); break; default: NOTIMPLEMENTED(); } callback.Run(std::move(sensor)); https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:356: bool gyroscope_sensor_is_available_; bool accelerometer_is_available_; bool linear_acceleration_sensor_is_available_; bool gyroscope_is_available_; https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:405: void SetUpOnIOThreadForAndroid() { This is the same as SetUpOnIOThread(). https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:246: return num_sensors_tried_ == kNumSensors; We can replace num_sensors_tried_ with the following check here: if (accelerometer_.sensor && !accelerometer_.shared_buffer) return false; if (linear_acceleration_sensor_.sensor && !linear_acceleration_sensor_.shared_buffer) { return false; } if (gyroscope_.sensor && !gyroscope_.shared_buffer) return false; return true; https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:42: protected: DeviceMotionEventPump has no subclasses so nothing needs to be protected. This can be in the private section. https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:86: SensorEntry accelerometer_sensor_; accelerometer_ https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:88: SensorEntry gyroscope_sensor_; gyroscope_ https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump_unittest.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump_unittest.cc:175: device::SensorReadingSharedBuffer* accelerometer_sensor_buffer_; accelerometer_buffer_ https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump_unittest.cc:177: device::SensorReadingSharedBuffer* gyroscope_sensor_buffer_; gyroscope_buffer_
Thanks for the fixes juncai@! In addition to Reilly's comments a couple of thoughts: If I understand correctly you want to move the Device Orientation logic to the renderer, there are a couple of issues that come to mind here: 1. Each renderer (roughly each new tab) would need to execute the fallback and related overhead, 2. If the device orientation api is used in two tabs that would result in transformations being executed multiple times resulting in unnecessary overhead, 3. Potential overhead with transformations If the buffer is read more frequently than updated by the platform, just to find out it hasn't changed sufficiently, 4. The fallback and sensor logic is specific to android and it would be better to keep 'platform' logic in the device layer, keeping the renderer more platform agnostic.. Therefore I think it may be sensible to keep most logic in the browser/device layer.. maybe have some special virtual (chrome specific) sensor (e.g. chrome_device_orientation/chrome_device_motion) on the device/ side and have a simple 'pump' mechanism in the renderer. wdyt? Arguably this is less relevant for device motion as it simply propagates the sensor data, however this may impact the overall design as motion and orientation are (historically) related.. Tomorrow (Tuesday) offsite here, so may be slow to reply. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:108: // method doesn't need to be implemented. On 2017/05/26 02:38:53, juncai wrote: > On 2017/05/24 15:30:16, timvolodine wrote: > > I guess this method shouldn't be called at all? > > It depends on the sensor reporting mode: > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... > It is called by the browser side to inform the renderer side that the sensor > data is updated: > https://cs.chromium.org/chromium/src/device/generic_sensor/sensor_impl.cc?l=5... > > But as the comments mentioned, since DeviceMotionEventPump::FireEvent() reads > the shared memory in a certain frequency, this method doesn't need to to do > anyting. Right, what I meant is that it could have a DCHECK or NOTREACHED() to enforce it is not called, as the reading is read using a dedicated timer. https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:190: device::SensorReading* result) { On 2017/05/26 02:38:52, juncai wrote: > On 2017/05/24 15:30:16, timvolodine wrote: > > maybe reader_ i.e. SharedMemorySeqlockReader can be used here? I think > > extracting the 'reading' functionality into a helper class would make it more > > readable/reusable.. > > The |shared_buffer| is a mojo::ScopedSharedBufferMapping > https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.h?l=37 > And the SharedMemorySeqlockReader needs to be initialized by a > base::SharedMemoryHandle > https://cs.chromium.org/chromium/src/content/renderer/shared_memory_seqlock_r... > I don't think the SharedMemorySeqlockReader can be used here. > > I moved this function and UpdateSensorReading() out of the SensorEntry struct > and make it more readable. > > Done. I see, maybe we could have something similar for the new mojo buffer to increase reusability..
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:108: // method doesn't need to be implemented. On 2017/05/30 00:53:29, timvolodine wrote: > On 2017/05/26 02:38:53, juncai wrote: > > On 2017/05/24 15:30:16, timvolodine wrote: > > > I guess this method shouldn't be called at all? > > > > It depends on the sensor reporting mode: > > > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... > > It is called by the browser side to inform the renderer side that the sensor > > data is updated: > > > https://cs.chromium.org/chromium/src/device/generic_sensor/sensor_impl.cc?l=5... > > > > But as the comments mentioned, since DeviceMotionEventPump::FireEvent() reads > > the shared memory in a certain frequency, this method doesn't need to to do > > anyting. > > Right, what I meant is that it could have a DCHECK or NOTREACHED() to enforce it > is not called, as the reading is read using a dedicated timer. I don't think a DCHECK() or NOTREACHED() can be used here, since this method is called if a sensor is on ON_CHANGE mode and our code doesn't set this mode. The sensor mode is a return value of the mojo call GetSensor(): https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:190: device::SensorReading* result) { On 2017/05/30 00:53:29, timvolodine wrote: > On 2017/05/26 02:38:52, juncai wrote: > > On 2017/05/24 15:30:16, timvolodine wrote: > > > maybe reader_ i.e. SharedMemorySeqlockReader can be used here? I think > > > extracting the 'reading' functionality into a helper class would make it > more > > > readable/reusable.. > > > > The |shared_buffer| is a mojo::ScopedSharedBufferMapping > > https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.h?l=37 > > And the SharedMemorySeqlockReader needs to be initialized by a > > base::SharedMemoryHandle > > > https://cs.chromium.org/chromium/src/content/renderer/shared_memory_seqlock_r... > > I don't think the SharedMemorySeqlockReader can be used here. > > > > I moved this function and UpdateSensorReading() out of the SensorEntry struct > > and make it more readable. > > > > Done. > > I see, maybe we could have something similar for the new mojo buffer to increase > reusability.. Filed an issue for this for a possible follow-up: https://bugs.chromium.org/p/chromium/issues/detail?id=727788 https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:156: class FakeAccelerometerSensor : public device::PlatformSensor { On 2017/05/27 03:17:50, Reilly Grant wrote: > "meter" means "sensor", just call it a FakeAccelerometer. Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:161: : PlatformSensor(type, std::move(mapping), provider) {} On 2017/05/27 03:17:50, Reilly Grant wrote: > type -> device::mojom::SensorType::ACCELEROMETER Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:179: void StopSensor() override{}; On 2017/05/27 03:17:50, Reilly Grant wrote: > nit: space before { There was a space between {, but it's odd that "git cl format" removes the space. Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:192: return default_configuration; On 2017/05/27 03:17:50, Reilly Grant wrote: > return device::PlatformSensorConfiguration(60 /* frequency */); Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:204: : PlatformSensor(type, std::move(mapping), provider) {} On 2017/05/27 03:17:50, Reilly Grant wrote: > type -> device::mojom::SensorType::LINEAR_ACCELERATION Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:235: return default_configuration; On 2017/05/27 03:17:50, Reilly Grant wrote: > return device::PlatformSensorConfiguration(60 /* frequency */); Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:242: class FakeGyroscopeSensor : public device::PlatformSensor { On 2017/05/27 03:17:50, Reilly Grant wrote: > "scope" also means "sensor", so just call it a FakeGyroscope. Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:247: : PlatformSensor(type, std::move(mapping), provider) {} On 2017/05/27 03:17:50, Reilly Grant wrote: > type -> device::mojom::SensorType::GYROSCOPE Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:278: return default_configuration; On 2017/05/27 03:17:50, Reilly Grant wrote: > return device::PlatformSensorConfiguration(60 /* frequency */); Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:351: } On 2017/05/27 03:17:50, Reilly Grant wrote: > scoped_refptr<device::PlatformSensor> sensor; > > switch (type) { > case device::mojom::SensorType::ACCELEROMETER: > if (accelerometer_is_available_) > sensor = new FakeAccelerometer(std::move(mapping), this); > break; > case device::mojom::SensorType::LINEAR_ACCELERATION: > if (linear_acceleration_sensor_is_available_) > sensor = new FakeLinearAccelerationSensor(std::move(mapping), this); > break; > case device::mojom::SensorType::GYROSCOPE: > if (gyroscope_is_available_) > sensor = new FakeGyroscope(std::move(mapping), this); > break; > default: > NOTIMPLEMENTED(); > } > > callback.Run(std::move(sensor)); Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:356: bool gyroscope_sensor_is_available_; On 2017/05/27 03:17:50, Reilly Grant wrote: > bool accelerometer_is_available_; > bool linear_acceleration_sensor_is_available_; > bool gyroscope_is_available_; Done. https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:405: void SetUpOnIOThreadForAndroid() { On 2017/05/27 03:17:50, Reilly Grant wrote: > This is the same as SetUpOnIOThread(). It doesn't have the SetUpFetcher(). https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.cc:246: return num_sensors_tried_ == kNumSensors; On 2017/05/27 03:17:51, Reilly Grant wrote: > We can replace num_sensors_tried_ with the following check here: > > if (accelerometer_.sensor && !accelerometer_.shared_buffer) > return false; > if (linear_acceleration_sensor_.sensor && > !linear_acceleration_sensor_.shared_buffer) { > return false; > } > if (gyroscope_.sensor && !gyroscope_.shared_buffer) > return false; > > return true; Done. https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:42: protected: On 2017/05/27 03:17:51, Reilly Grant wrote: > DeviceMotionEventPump has no subclasses so nothing needs to be protected. This > can be in the private section. The DeviceMotionEventPumpForTesting is a subclass of DeviceMotionEventPump and it needs to access the protected fields. https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:86: SensorEntry accelerometer_sensor_; On 2017/05/27 03:17:51, Reilly Grant wrote: > accelerometer_ Done. https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump.h:88: SensorEntry gyroscope_sensor_; On 2017/05/27 03:17:51, Reilly Grant wrote: > gyroscope_ Done. https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... File content/renderer/device_sensors/device_motion_event_pump_unittest.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump_unittest.cc:175: device::SensorReadingSharedBuffer* accelerometer_sensor_buffer_; On 2017/05/27 03:17:51, Reilly Grant wrote: > accelerometer_buffer_ Done. https://codereview.chromium.org/2896583005/diff/80001/content/renderer/device... content/renderer/device_sensors/device_motion_event_pump_unittest.cc:177: device::SensorReadingSharedBuffer* gyroscope_sensor_buffer_; On 2017/05/27 03:17:51, Reilly Grant wrote: > gyroscope_buffer_ Done.
On 2017/05/30 00:53:30, timvolodine wrote: > Thanks for the fixes juncai@! In addition to Reilly's comments a couple of > thoughts: > > If I understand correctly you want to move the Device Orientation logic to the > renderer, there are a couple of issues that come to mind here: > 1. Each renderer (roughly each new tab) would need to execute the fallback and > related overhead, > 2. If the device orientation api is used in two tabs that would result in > transformations being executed multiple times resulting in unnecessary overhead, On the device motion and orientation spec, it says "fire events only on the top-level browsing context", so tabs that are not currently active are not listening to the device motion/orientation event, and will not have the unnecessary transformation overhead. > 3. Potential overhead with transformations If the buffer is read more frequently > than updated by the platform, just to find out it hasn't changed sufficiently, Since each sensor updates its data independently, it makes sense to read all their data at the same time in a certain frequency since other sensor may change more frequently, even though some sensor may change less frequently. > 4. The fallback and sensor logic is specific to android and it would be better > to keep 'platform' logic in the device layer, keeping the renderer more platform > agnostic.. The current Mac implementation of device orientation also uses some kind of fallback by using accelerometer: https://cs.chromium.org/chromium/src/device/sensors/data_fetcher_shared_memor... I think it is better to apply this kind of fallback so that we don't have to deal with platform-specific implementation. Maybe in the device orientation implementation, it needs a 4th fallback which can calculate the orientation data only from ACCELEROMETER. > > Therefore I think it may be sensible to keep most logic in the browser/device > layer.. maybe have some special virtual (chrome specific) sensor (e.g. > chrome_device_orientation/chrome_device_motion) on the device/ side and have a > simple 'pump' mechanism in the renderer. wdyt? This transformation logic is not very related to the //device/generic_sensor, and given the reasons I mentioned above, the transformation computation overhead is not that much, it seems that moving it there is not ideal. > > Arguably this is less relevant for device motion as it simply propagates the > sensor data, however this may impact the overall design as motion and > orientation are (historically) related.. > > Tomorrow (Tuesday) offsite here, so may be slow to reply.
On 2017/05/30 22:27:41, juncai wrote: > On 2017/05/30 00:53:30, timvolodine wrote: > > Thanks for the fixes juncai@! In addition to Reilly's comments a couple of > > thoughts: > > > > If I understand correctly you want to move the Device Orientation logic to the > > renderer, there are a couple of issues that come to mind here: > > 1. Each renderer (roughly each new tab) would need to execute the fallback and > > related overhead, > > 2. If the device orientation api is used in two tabs that would result in > > transformations being executed multiple times resulting in unnecessary > overhead, > > On the device motion and orientation spec, it says "fire events only on the > top-level browsing context", so tabs that are not currently active are not > listening to the device motion/orientation event, and will not have the > unnecessary transformation overhead. To clarify my above comments, what I meant is if a tab is not visible it will not fire the device motion/orientation event: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/30 22:27:41, juncai wrote: > On 2017/05/30 00:53:30, timvolodine wrote: > > Thanks for the fixes juncai@! In addition to Reilly's comments a couple of > > thoughts: > > > > If I understand correctly you want to move the Device Orientation logic to the > > renderer, there are a couple of issues that come to mind here: > > 1. Each renderer (roughly each new tab) would need to execute the fallback and > > related overhead, > > 2. If the device orientation api is used in two tabs that would result in > > transformations being executed multiple times resulting in unnecessary > overhead, > > On the device motion and orientation spec, it says "fire events only on the > top-level browsing context", so tabs that are not currently active are not > listening to the device motion/orientation event, and will not have the > unnecessary transformation overhead. There could be multiple visible tabs, e.g. in recent versions of Android there is support for multi-window. > > > 3. Potential overhead with transformations If the buffer is read more > frequently > > than updated by the platform, just to find out it hasn't changed sufficiently, > > Since each sensor updates its data independently, it makes sense to read all > their data at the same time in a certain frequency since other sensor may change > more frequently, even though some sensor may change less frequently. > > > 4. The fallback and sensor logic is specific to android and it would be better > > to keep 'platform' logic in the device layer, keeping the renderer more > platform > > agnostic.. > > The current Mac implementation of device orientation also uses some kind of > fallback by using accelerometer: > https://cs.chromium.org/chromium/src/device/sensors/data_fetcher_shared_memor... > I think it is better to apply this kind of fallback so that we don't have to > deal with platform-specific implementation. > Maybe in the device orientation implementation, it needs a 4th fallback which > can calculate the orientation data only from ACCELEROMETER. > > > > > Therefore I think it may be sensible to keep most logic in the browser/device > > layer.. maybe have some special virtual (chrome specific) sensor (e.g. > > chrome_device_orientation/chrome_device_motion) on the device/ side and have a > > simple 'pump' mechanism in the renderer. wdyt? > > This transformation logic is not very related to the //device/generic_sensor, > and given the reasons I mentioned above, the transformation computation overhead > is not that much, it seems that moving it there is not ideal. > I am not sure I understand the reasons why the hardware and sensor specific bits should move to the renderer process? Maybe you could summarize them a bit more? I still think the hardware and sensor specific bits including transformations and sensor fallbacks should live in the browser e.g. device/. There is no reason for the renderer to perform platform specific bits resulting in potential redundancy and overhead. It seems this could be done by the backend (device/generic_sensor) which deals with the actual hardware sensors and their platform variations. Originally the "pump" mechanism was designed to simply read the shared memory and propagate into blink (shared memory has the benefit of not replicating the data/communication across renderers anyway). In that context it also seems more logical to perform sensor transformations "on-write" instead of "on-read".
On 2017/05/31 16:58:24, timvolodine wrote: > There could be multiple visible tabs, e.g. in recent versions of Android there > is support for multi-window. I did a quick test for multiple Chrome tabs on Android N, and even though two tabs are both visible, only one tab is getting the device motion/orientation data. So moving the transformation logic to renderer won't cause extra overhead. And the transformation logic is some math operation and is not computationally extensive. > I am not sure I understand the reasons why the hardware and sensor specific bits > should move to the renderer process? Maybe you could summarize them a bit more? > > I still think the hardware and sensor specific bits including transformations > and sensor fallbacks should live in the browser e.g. device/. There is no reason > for the renderer to perform platform specific bits resulting in potential > redundancy and overhead. It seems this could be done by the backend > (device/generic_sensor) which deals with the actual hardware sensors and their > platform variations. Originally the "pump" mechanism was designed to simply read > the shared memory and propagate into blink (shared memory has the benefit of not > replicating the data/communication across renderers anyway). In that context it > also seems more logical to perform sensor transformations "on-write" instead of > "on-read". The device motion/orientation event is not generic since it needs to use some combined sensor data, so I don't think //device/generic_sensor is an ideal place for it. The //device/generic_sensor provides an abstraction for sensors that can be used for some specific purposes, and in this case, motion/orientation. And in the future, imagine some new Web APIs need other sensor combination and if we add all those combination/transformation logic to //device/generic_sensor, it becomes not generic any more. That's why we would like to move those specific combination/transformation logic to renderer side, which is Web API specific computation and no other places use them except the Web API itself.
On 2017/05/31 18:43:09, juncai wrote: > On 2017/05/31 16:58:24, timvolodine wrote: > > There could be multiple visible tabs, e.g. in recent versions of Android there > > is support for multi-window. > > I did a quick test for multiple Chrome tabs on Android N, and even though two > tabs are both visible, only one tab is getting the device motion/orientation > data. So moving the transformation logic to renderer won't cause extra overhead. > And the transformation logic is some math operation and is not computationally > extensive. That looks like a bug? Other platforms should be considered as well, macOS, chromeOS.. > > > I am not sure I understand the reasons why the hardware and sensor specific > bits > > should move to the renderer process? Maybe you could summarize them a bit > more? > > > > I still think the hardware and sensor specific bits including transformations > > and sensor fallbacks should live in the browser e.g. device/. There is no > reason > > for the renderer to perform platform specific bits resulting in potential > > redundancy and overhead. It seems this could be done by the backend > > (device/generic_sensor) which deals with the actual hardware sensors and their > > platform variations. Originally the "pump" mechanism was designed to simply > read > > the shared memory and propagate into blink (shared memory has the benefit of > not > > replicating the data/communication across renderers anyway). In that context > it > > also seems more logical to perform sensor transformations "on-write" instead > of > > "on-read". > > The device motion/orientation event is not generic since it needs to use some > combined sensor data, so I don't think //device/generic_sensor is an ideal place > for it. The //device/generic_sensor provides an abstraction for sensors that can > be used for some specific purposes, and in this case, motion/orientation. And in > the future, imagine some new Web APIs need other sensor combination and if we > add all those combination/transformation logic to //device/generic_sensor, it > becomes not generic any more. That's why we would like to move those specific > combination/transformation logic to renderer side, which is Web API specific > computation and no other places use them except the Web API itself. On some platforms like windows there is direct support for orientation (via SENSOR_TYPE_INCLINOMETER_3D) so in that case there is no need for fallback/transformation.. Not sure if it is supported by generic_sensor/ at this stage (looks like it's not).. Also the fallback/sensor selection logic doesn't have to live directly in device/generic_sensor, it's just that the renderer doesn't seem to be the best place for it, because it's platform specific. Combining sensors is a pretty common situation (i.e. sensor fusion) and e.g. platforms like android and windows provide some 'virtual' sensors defined in terms of the basic ones. One of the examples is the TYPE_ROTATION_VECTOR sensor which is used for ABSOLUTE_ORIENTATION. Actually, looking at the mojo interface for generic_sensor the whole thing boils down to how SensorType.RELATIVE_ORIENTATION and SensorType.ABSOLUTE_ORIENTATION are implemented, which are not real sensors hence the potential for confusion. To match the existing orientation implementation the 'generic' RELATIVE_ORIENTATION could provide the required fallback mechanism on android for example. Alternatively there could be a dedicated type like '(CHROME_)ORIENTATION' using Euler angles or/and there could be wrappers around generic sensors (e.g. virtual/ sub-directory), which would also make it easier to implement any future APIs requiring non-trivial sensor usage instead of 'hacking' the renderer.
On 2017/06/02 12:56:19, timvolodine wrote: > On some platforms like windows there is direct support for orientation (via > SENSOR_TYPE_INCLINOMETER_3D) so in that case there is no need for > fallback/transformation.. Not sure if it is supported by generic_sensor/ at this > stage (looks like it's not).. Also the fallback/sensor selection logic doesn't > have to live directly in device/generic_sensor, it's just that the renderer > doesn't seem to be the best place for it, because it's platform specific. > Combining sensors is a pretty common situation (i.e. sensor fusion) and e.g. > platforms like android and windows provide some 'virtual' sensors defined in > terms of the basic ones. One of the examples is the TYPE_ROTATION_VECTOR sensor > which is used for ABSOLUTE_ORIENTATION. > > Actually, looking at the mojo interface for generic_sensor the whole thing boils > down to how SensorType.RELATIVE_ORIENTATION and SensorType.ABSOLUTE_ORIENTATION > are implemented, which are not real sensors hence the potential for confusion. > To match the existing orientation implementation the 'generic' > RELATIVE_ORIENTATION could provide the required fallback mechanism on android > for example. Alternatively there could be a dedicated type like > '(CHROME_)ORIENTATION' using Euler angles or/and there could be wrappers around > generic sensors (e.g. virtual/ sub-directory), which would also make it easier > to implement any future APIs requiring non-trivial sensor usage instead of > 'hacking' the renderer. Hi timvolodine@, thanks for the thoughts. Yes, refactoring how SensorType.RELATIVE_ORIENTATION and SensorType.ABSOLUTE_ORIENTATION are implemented in //device/generic_sensor seems a good way to move the fallback logic on certain platforms from renderer to browser so that the device orientation renderer code doesn't need to do that. So here is the plan: 1. On Android, similar fallback can be implemented for ABSOLUTE_ORIENTATION: a. ROTATION_VECTOR b. combination of ACCELEROMETER and MAGNETIC_FIELD 2. On Windows, SENSOR_TYPE_INCLINOMETER_3D can be used to implement RELATIVE_ORIENTATION. After the above refactoring is done, the device orientation pump only needs to access two sensors: RELATIVE_ORIENTATION and ABSOLUTE_ORIENTATION. And if RELATIVE_ORIENTATION sensor can provide data, then use it; otherwise fallback to ABSOLUTE_ORIENTATION sensor. And device orientation absolute pump just needs to use ABSOLUTE_ORIENTATION sensor. Note that both the RELATIVE_ORIENTATION and ABSOLUTE_ORIENTATION return quaternion data, and it needs to be transformed to angles which are used by device orientation spec. What do you think?
On 2017/06/02 18:33:47, juncai wrote: > On 2017/06/02 12:56:19, timvolodine wrote: > > On some platforms like windows there is direct support for orientation (via > > SENSOR_TYPE_INCLINOMETER_3D) so in that case there is no need for > > fallback/transformation.. Not sure if it is supported by generic_sensor/ at > this > > stage (looks like it's not).. Also the fallback/sensor selection logic doesn't > > have to live directly in device/generic_sensor, it's just that the renderer > > doesn't seem to be the best place for it, because it's platform specific. > > Combining sensors is a pretty common situation (i.e. sensor fusion) and e.g. > > platforms like android and windows provide some 'virtual' sensors defined in > > terms of the basic ones. One of the examples is the TYPE_ROTATION_VECTOR > sensor > > which is used for ABSOLUTE_ORIENTATION. > > > > Actually, looking at the mojo interface for generic_sensor the whole thing > boils > > down to how SensorType.RELATIVE_ORIENTATION and > SensorType.ABSOLUTE_ORIENTATION > > are implemented, which are not real sensors hence the potential for confusion. > > To match the existing orientation implementation the 'generic' > > RELATIVE_ORIENTATION could provide the required fallback mechanism on android > > for example. Alternatively there could be a dedicated type like > > '(CHROME_)ORIENTATION' using Euler angles or/and there could be wrappers > around > > generic sensors (e.g. virtual/ sub-directory), which would also make it easier > > to implement any future APIs requiring non-trivial sensor usage instead of > > 'hacking' the renderer. > > Hi timvolodine@, thanks for the thoughts. Yes, refactoring how > SensorType.RELATIVE_ORIENTATION and SensorType.ABSOLUTE_ORIENTATION are > implemented in //device/generic_sensor seems a good way to move the fallback > logic on certain platforms from renderer to browser so that the device > orientation renderer code doesn't need to do that. So here is the plan: > 1. On Android, similar fallback can be implemented for ABSOLUTE_ORIENTATION: > a. ROTATION_VECTOR > b. combination of ACCELEROMETER and MAGNETIC_FIELD > 2. On Windows, SENSOR_TYPE_INCLINOMETER_3D can be used to implement > RELATIVE_ORIENTATION. > > After the above refactoring is done, the device orientation pump only needs to > access two sensors: RELATIVE_ORIENTATION and ABSOLUTE_ORIENTATION. And if > RELATIVE_ORIENTATION sensor can provide data, then use it; otherwise fallback to > ABSOLUTE_ORIENTATION sensor. And device orientation absolute pump just needs to > use ABSOLUTE_ORIENTATION sensor. > > Note that both the RELATIVE_ORIENTATION and ABSOLUTE_ORIENTATION return > quaternion data, and it needs to be transformed to angles which are used by > device orientation spec. > > What do you think? Sounds good. Thanks for constructively keeping up with this. The fallback from relative to absolute seems clean and is looks equivalent to the 3-way fallback we have now. Also I think we should make sure the platform bits from device/sensors are somehow supported in the new framework, in particular the windows and chrome_os implementations. This to make sure the whole thing remains compatible. Another thought: AFAIK the RELATIVE_ORIENTATION and ABSOLUTE_ORIENTATION 'sensors' are currently only used (or planned to be used) by the Device Orientation API. While the quaternion representation is nice I don't think there is any reason to prefer it to euler angles at this point for the sake of implementation. In fact using euler angles would likely provide a more efficient implementation which could be written directly into the shared memory without subsequent transformations. In particular the SENSOR_TYPE_INCLINOMETER_3D comes to mind which provides Euler angles directly and would need to be transformed to quaternion and then back-transformed to Euler angles in the current framework. I seems the discussion on this patch got a bit side-tracked. So coming back to Device Motion: initially I thought you may get a simpler implementation by introducing something like a DEVICE_MOTION sensor on the backend side. However looking at it now it seems the advantage would be limited and largely equivalent, so I'll leave it up to you to decide on this issue. I think we can proceed with this patch. What still bothers me somewhat is the reintroduction of the WebDeviceMotionData. I think it would be good provide some argumentation as to why this necessary and why this is better than reusing the existing device::MotionData (in the context of servicification).
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/02 23:21:38, timvolodine wrote: > Sounds good. Thanks for constructively keeping up with this. The fallback from > relative to absolute seems clean and is looks equivalent to the 3-way fallback > we have now. Also I think we should make sure the platform bits from > device/sensors are somehow supported in the new framework, in particular the > windows and chrome_os implementations. This to make sure the whole thing remains > compatible. > > Another thought: AFAIK the RELATIVE_ORIENTATION and ABSOLUTE_ORIENTATION > 'sensors' are currently only used (or planned to be used) by the Device > Orientation API. While the quaternion representation is nice I don't think there > is any reason to prefer it to euler angles at this point for the sake of > implementation. In fact using euler angles would likely provide a more efficient > implementation which could be written directly into the shared memory without > subsequent transformations. In particular the SENSOR_TYPE_INCLINOMETER_3D comes > to mind which provides Euler angles directly and would need to be transformed to > quaternion and then back-transformed to Euler angles in the current framework. > > I seems the discussion on this patch got a bit side-tracked. So coming back to > Device Motion: initially I thought you may get a simpler implementation by > introducing something like a DEVICE_MOTION sensor on the backend side. However > looking at it now it seems the advantage would be limited and largely > equivalent, so I'll leave it up to you to decide on this issue. I think we can > proceed with this patch. What still bothers me somewhat is the reintroduction of > the WebDeviceMotionData. I think it would be good provide some argumentation as > to why this necessary and why this is better than reusing the existing > device::MotionData (in the context of servicification). Thanks for the comments! I removed the WebDeviceMotionData and currently still use //device/sensors/motion_data.h. After device orientation refactoring is done, //device/sensors/public/cpp/motion_data.* and //device/sensors/public/cpp/orientation_data.* can be moved to //device/generic_sensor/public/cpp/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
juncai@chromium.org changed reviewers: + jam@chromium.org
timvolodine@, reillyg@: Please take a look again on the device sensor related changes. jam@chromium.org: Please review changes in //content/renderer/BUILD.gn //content/renderer/DEPS //content/test/DEPS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/06/02 23:21:38, timvolodine wrote: > [...] What still bothers me somewhat is the reintroduction of > the WebDeviceMotionData. I think it would be good provide some argumentation as > to why this necessary and why this is better than reusing the existing > device::MotionData (in the context of servicification). The re-introduction of WebDeviceMotionData is a temporary measure to remove the dependency on //device/sensors because that is what we are trying to delete. WebDeviceMotionData can be removed when //content/renderer/device_sensors is merged into Blink (Onion Soup).
https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:405: void SetUpOnIOThreadForAndroid() { On 2017/05/30 22:26:57, juncai wrote: > On 2017/05/27 03:17:50, Reilly Grant wrote: > > This is the same as SetUpOnIOThread(). > > It doesn't have the SetUpFetcher(). It would be easier to read if you put the call to SetUpFetcher() inside an #if defined(OS_ANDROID) instead of duplicating all the other code with this small difference. https://codereview.chromium.org/2896583005/diff/160001/content/browser/device... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/160001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:291: gyroscope_is_available_(true) {} nit: It's cleaner to do these default initializations below: bool accelerometer_is_available_ = true; bool linear_acceleration_sensor_is_available_ = true; bool gyroscope_is_available_ = true; https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:53: pump_delay_microseconds_(kDefaultPumpDelayMicroseconds), This is a constant so it doesn't need to be a field. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:110: gyroscope_.HandleSensorError(); I think this method should use Sensor::Suspend() instead of closing all the pipes. This method gets called both when the page visibility changes and if the last event listener is unregistered. My assumption is that removing the event listener is more rare than the page visibility changing so we should optimize for not doing extra work in this case. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:124: client_binding.Close(); This happens implicitly when the object is destroyed. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:154: DCHECK(sensor.is_bound()); I think you probably mean DCHECK(!client_binding.is_bound()), though that check would be redundant since there is a similar one inside mojo::Binding::Bind. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:176: DCHECK_GE(kMaxAllowedFrequency, frequency_limits.second); frequency_limits doesn't seem to be used outside of these checks, which don't seem necessary. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:178: sensor->AddConfiguration(default_config, We want to ask for 60Hz here. Is it guaranteed that that is the default reported by the browser process?
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/06 20:00:13, Reilly Grant wrote: > On 2017/06/02 23:21:38, timvolodine wrote: > > [...] What still bothers me somewhat is the reintroduction of > > the WebDeviceMotionData. I think it would be good provide some argumentation > as > > to why this necessary and why this is better than reusing the existing > > device::MotionData (in the context of servicification). > > The re-introduction of WebDeviceMotionData is a temporary measure to remove the > dependency on //device/sensors because that is what we are trying to delete. > WebDeviceMotionData can be removed when //content/renderer/device_sensors is > merged into Blink (Onion Soup). Discussed this with reillyg@ offline and came to the conclusion that since the //device/sensors/public/cpp/motion_data.h will be removed eventually, and if replacing it with a temporary WebDeviceMotionData and it also will be removed eventually, it makes sense for now to just continue using //device/sensors/public/cpp/motion_data.h until it is time to remove it. This also makes the patch smaller since much less files need to modified.
https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/80001/content/browser/device_... content/browser/device_sensors/device_sensor_browsertest.cc:405: void SetUpOnIOThreadForAndroid() { On 2017/06/06 20:58:55, Reilly Grant wrote: > On 2017/05/30 22:26:57, juncai wrote: > > On 2017/05/27 03:17:50, Reilly Grant wrote: > > > This is the same as SetUpOnIOThread(). > > > > It doesn't have the SetUpFetcher(). > > It would be easier to read if you put the call to SetUpFetcher() inside an #if > defined(OS_ANDROID) instead of duplicating all the other code with this small > difference. Done. https://codereview.chromium.org/2896583005/diff/160001/content/browser/device... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/160001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:291: gyroscope_is_available_(true) {} On 2017/06/06 20:58:55, Reilly Grant wrote: > nit: It's cleaner to do these default initializations below: > > bool accelerometer_is_available_ = true; > bool linear_acceleration_sensor_is_available_ = true; > bool gyroscope_is_available_ = true; Done. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:53: pump_delay_microseconds_(kDefaultPumpDelayMicroseconds), On 2017/06/06 20:58:55, Reilly Grant wrote: > This is a constant so it doesn't need to be a field. Done. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:110: gyroscope_.HandleSensorError(); On 2017/06/06 20:58:55, Reilly Grant wrote: > I think this method should use Sensor::Suspend() instead of closing all the > pipes. This method gets called both when the page visibility changes and if the > last event listener is unregistered. My assumption is that removing the event > listener is more rare than the page visibility changing so we should optimize > for not doing extra work in this case. Done. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:124: client_binding.Close(); On 2017/06/06 20:58:55, Reilly Grant wrote: > This happens implicitly when the object is destroyed. Done. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:154: DCHECK(sensor.is_bound()); On 2017/06/06 20:58:55, Reilly Grant wrote: > I think you probably mean DCHECK(!client_binding.is_bound()), though that check > would be redundant since there is a similar one inside mojo::Binding::Bind. It is similar to the following code: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/sensor... https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:176: DCHECK_GE(kMaxAllowedFrequency, frequency_limits.second); On 2017/06/06 20:58:55, Reilly Grant wrote: > frequency_limits doesn't seem to be used outside of these checks, which don't > seem necessary. Done. https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:178: sensor->AddConfiguration(default_config, On 2017/06/06 20:58:55, Reilly Grant wrote: > We want to ask for 60Hz here. Is it guaranteed that that is the default reported > by the browser process? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/06/05 20:02:40, juncai wrote: > timvolodine@, reillyg@: Please take a look again on the device sensor related > changes. > > mailto:jam@chromium.org: Please review changes in > //content/renderer/BUILD.gn > //content/renderer/DEPS > //content/test/DEPS lgtm
ping timvolodine, :).
Thanks juncai, a few more questions/comments below. Didn't look at all the details but looks ok overall. https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:354: #endif // defined(OS_ANDROID) same question here, no #else? https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:379: SetUpFetcher(); why do we need to set-up the fetcher only on non-android platforms? what happens on android? https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:500: // expects to get an event with only some fake values, because only nit: maybe be more specific "..expects to get an event with only the gyroscope and linear accelerometer values, because no accelerometer values can be provided." https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:19: bool TryReadFromBuffer(const device::SensorReadingSharedBuffer* buffer, These methods and everything related to reading can perhaps move to a dedicated (potentially templated) reader class similar to SharedMemorySeqLockReader, so that it could be reused for the orientation implementation. Fine if this happens in a follow-up, but please add a TODO then. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:94: !RenderThreadImpl::current()->layout_test_mode()) { nit: maybe just do an early return in case it's in test mode, would be easier to read I think. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:122: accelerometer_.sensor->Suspend(); What happens when we close a window, will that stop the sensors by breaking the pipe? Does this also mean that if I unregister all 'ondevicemotion' handlers the sensors will be in suspended state? (does this result in some resources being held?) https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:150: // method doesn't need to be implemented. sorry if I asked this previously, can we put NOTREACHED() here? (since I assume we read the buffer ourselves and don't need this signal) https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:188: device::mojom::SensorConfiguration::kMaxAllowedFrequency; nit: probably no need for constexpr? https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.h:95: enum class PumpState { STOPPED, RUNNING, PENDING_START }; This is functionality from the device_sensor_event_pump base class originally (with Start and Stop methods already implemented there). Can DeviceSensorEventPump be re-used here? https://codereview.chromium.org/2896583005/diff/180001/content/test/data/devi... File content/test/data/device_sensors/device_motion_test.html (right): https://codereview.chromium.org/2896583005/diff/180001/content/test/data/devi... content/test/data/device_sensors/device_motion_test.html:5: let expectedInterval = Math.floor(1000000 / 60); is this in microseconds? the device orientation spec states that interval should be in milliseconds. https://codereview.chromium.org/2896583005/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2896583005/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:60: // features::kGenericSensor flag. nit: maybe add a TODO to remove when and if generic sensors is live? https://codereview.chromium.org/2896583005/diff/180001/services/device/device... File services/device/device_service.cc (left): https://codereview.chromium.org/2896583005/diff/180001/services/device/device... services/device/device_service.cc:167: void DeviceService::BindMotionSensorRequest( Does this mean we'll run everything on UI thread?
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:354: #endif // defined(OS_ANDROID) On 2017/06/07 20:37:35, timvolodine wrote: > same question here, no #else? As the comment above and inside SetUpOnIOThread() says, on Android, the DeviceSensorService lives on the UI thread, and on non-Android platforms, the DeviceSensorService lives on the IO thread. And SetUpFetcher() calls device::DeviceSensorService::GetInstance()->SetDataFetcherForTesting(fetcher_); So it needs a #if defined(OS_ANDROID) to call SetUpFetcher() on Android. And sensor_provider_ = FakeSensorProvider::GetInstance(); device::PlatformSensorProvider::SetProviderForTesting(sensor_provider_); io_loop_finished_event_.Signal(); are called on IO thread for all platforms. So no #else here so it can be reused on all platforms. https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:379: SetUpFetcher(); On 2017/06/07 20:37:35, timvolodine wrote: > why do we need to set-up the fetcher only on non-android platforms? what happens > on android? On Android, SetUpFetcher() is called at the above SetUpOnMainThread() with #if defined(OS_ANDROID) since it needs to live on the UI thread. https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:500: // expects to get an event with only some fake values, because only On 2017/06/07 20:37:35, timvolodine wrote: > nit: maybe be more specific "..expects to get an event with only the gyroscope > and linear accelerometer values, because no accelerometer values can be > provided." Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:19: bool TryReadFromBuffer(const device::SensorReadingSharedBuffer* buffer, On 2017/06/07 20:37:36, timvolodine wrote: > These methods and everything related to reading can perhaps move to a dedicated > (potentially templated) reader class similar to SharedMemorySeqLockReader, so > that it could be reused for the orientation implementation. Fine if this happens > in a follow-up, but please add a TODO then. Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:94: !RenderThreadImpl::current()->layout_test_mode()) { On 2017/06/07 20:37:35, timvolodine wrote: > nit: maybe just do an early return in case it's in test mode, would be easier to > read I think. Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:122: accelerometer_.sensor->Suspend(); On 2017/06/07 20:37:35, timvolodine wrote: > What happens when we close a window, will that stop the sensors by breaking the > pipe? Does this also mean that if I unregister all 'ondevicemotion' handlers the > sensors will be in suspended state? (does this result in some resources being > held?) When a window is closed, the pipe is closed and the sensors are stopped. If all device motion event listener are unregistered, the sensors will be in a suspended state. Related comments about using Resume() and Suspend() by reillyg@ is at: https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:150: // method doesn't need to be implemented. On 2017/06/07 20:37:36, timvolodine wrote: > sorry if I asked this previously, can we put NOTREACHED() here? (since I assume > we read the buffer ourselves and don't need this signal) I don't think NOREACHED() can be put here. This question was asked and answered here: https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... I copied the answer below: I don't think a DCHECK() or NOTREACHED() can be used here, since this method is called if a sensor is on ON_CHANGE mode and our code doesn't set this mode. The sensor mode is a return value of the mojo call GetSensor(): https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:188: device::mojom::SensorConfiguration::kMaxAllowedFrequency; On 2017/06/07 20:37:35, timvolodine wrote: > nit: probably no need for constexpr? Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.h:95: enum class PumpState { STOPPED, RUNNING, PENDING_START }; On 2017/06/07 20:37:36, timvolodine wrote: > This is functionality from the device_sensor_event_pump base class originally > (with Start and Stop methods already implemented there). Can > DeviceSensorEventPump be re-used here? Yes, it will be re-used eventually when the device orientation is refactored. Similar to the previously closed bigger CL: https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... For now, I think it makes sense to have some duplicate code here. It makes this CL cleaner by not modifying any orientation related code since this CL makes DeviceMotionEventPump and DeviceOrientationEventPump not share DeviceSensorEventPump code. https://codereview.chromium.org/2896583005/diff/180001/content/test/data/devi... File content/test/data/device_sensors/device_motion_test.html (right): https://codereview.chromium.org/2896583005/diff/180001/content/test/data/devi... content/test/data/device_sensors/device_motion_test.html:5: let expectedInterval = Math.floor(1000000 / 60); On 2017/06/07 20:37:36, timvolodine wrote: > is this in microseconds? the device orientation spec states that interval should > be in milliseconds. Yes, it should be in milliseconds. Thanks! Done. https://codereview.chromium.org/2896583005/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2896583005/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:60: // features::kGenericSensor flag. On 2017/06/07 20:37:36, timvolodine wrote: > nit: maybe add a TODO to remove when and if generic sensors is live? Done. https://codereview.chromium.org/2896583005/diff/180001/services/device/device... File services/device/device_service.cc (left): https://codereview.chromium.org/2896583005/diff/180001/services/device/device... services/device/device_service.cc:167: void DeviceService::BindMotionSensorRequest( On 2017/06/07 20:37:36, timvolodine wrote: > Does this mean we'll run everything on UI thread? Since now the device motion depends only on //device/generic_sensor and it uses: https://cs.chromium.org/chromium/src/services/device/device_service.cc?type=c... And it runs on IO thread.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
also, looking at generic_sensors they still seem to be experimental, landing this patch would switch motion (which is stable) to the experimental implementation (at least partially) - any concerns? https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:379: SetUpFetcher(); On 2017/06/07 23:05:07, juncai wrote: > On 2017/06/07 20:37:35, timvolodine wrote: > > why do we need to set-up the fetcher only on non-android platforms? what > happens > > on android? > > On Android, SetUpFetcher() is called at the above SetUpOnMainThread() with #if > defined(OS_ANDROID) since it needs to live on the UI thread. oh I see, it's because you need to do extra stuff now on the IO thread on all platforms like setting sensor_provider_.. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:122: accelerometer_.sensor->Suspend(); On 2017/06/07 23:05:07, juncai wrote: > On 2017/06/07 20:37:35, timvolodine wrote: > > What happens when we close a window, will that stop the sensors by breaking > the > > pipe? Does this also mean that if I unregister all 'ondevicemotion' handlers > the > > sensors will be in suspended state? (does this result in some resources being > > held?) > > When a window is closed, the pipe is closed and the sensors are stopped. If all > device motion event listener are unregistered, the sensors will be in a > suspended state. > > Related comments about using Resume() and Suspend() by reillyg@ is at: > https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... I see, so it's optimized for visibility change. Perhaps add a comment to explain this (something similar to what Reilly said). https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:150: // method doesn't need to be implemented. On 2017/06/07 23:05:07, juncai wrote: > On 2017/06/07 20:37:36, timvolodine wrote: > > sorry if I asked this previously, can we put NOTREACHED() here? (since I > assume > > we read the buffer ourselves and don't need this signal) > > I don't think NOREACHED() can be put here. > > This question was asked and answered here: > https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... > > I copied the answer below: > I don't think a DCHECK() or NOTREACHED() can be used here, since this method is > called if a sensor is on ON_CHANGE mode and our code doesn't set this mode. The > sensor mode is a return value of the mojo call GetSensor(): > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... This seems strange - we would get unnecessary high frequency mojo IPC calls? maybe we should set the mode to explicitly reflect the shared memory usage (I thought it was supported in generic_sensors/)? https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:188: device::mojom::SensorConfiguration::kMaxAllowedFrequency; On 2017/06/07 23:05:07, juncai wrote: > On 2017/06/07 20:37:35, timvolodine wrote: > > nit: probably no need for constexpr? > > Done. nit: I actually meant putting device::mojom::SensorConfiguration::kMaxAllowedFrequency directly inside the DCHECK_GE (if it's not used anywhere else) https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.h:95: enum class PumpState { STOPPED, RUNNING, PENDING_START }; On 2017/06/07 23:05:07, juncai wrote: > On 2017/06/07 20:37:36, timvolodine wrote: > > This is functionality from the device_sensor_event_pump base class originally > > (with Start and Stop methods already implemented there). Can > > DeviceSensorEventPump be re-used here? > > Yes, it will be re-used eventually when the device orientation is refactored. > Similar to the previously closed bigger CL: > https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... > > For now, I think it makes sense to have some duplicate code here. It makes this > CL cleaner by not modifying any orientation related code since this CL makes > DeviceMotionEventPump and DeviceOrientationEventPump not share > DeviceSensorEventPump code. I though this CL is only about motion? I was wondering if it's possible to reuse without touching base class or orientation..
On 2017/06/08 20:26:51, timvolodine wrote: > also, looking at generic_sensors they still seem to be experimental, landing > this patch would switch motion (which is stable) to the experimental > implementation (at least partially) - any concerns? There are some advantages of switching the sensor backend to //device/generic_sensor: 1. //device/generic_sensor/ has better defined mojo interfaces which are more flexible. 2. //device/generic_sensor/ has all the sensor types that Chrome needs instead of spreading the sensor implementation into two places. 3. From device servicification point of view, //device/generic_sensor is very close to be servicified, and if we switch the device motion/orientation backend to //device/generic_sensor, then //device/sensors/ doesn't need to be servicified since it will be removed. There is disadvantage of this as you said, the generic_sensor is still experimental. But I think the advantage seems much larger than the disadvantage.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... File content/browser/device_sensors/device_sensor_browsertest.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/browser/device... content/browser/device_sensors/device_sensor_browsertest.cc:379: SetUpFetcher(); On 2017/06/08 20:26:51, timvolodine wrote: > On 2017/06/07 23:05:07, juncai wrote: > > On 2017/06/07 20:37:35, timvolodine wrote: > > > why do we need to set-up the fetcher only on non-android platforms? what > > happens > > > on android? > > > > On Android, SetUpFetcher() is called at the above SetUpOnMainThread() with #if > > defined(OS_ANDROID) since it needs to live on the UI thread. > > oh I see, it's because you need to do extra stuff now on the IO thread on all > platforms like setting sensor_provider_.. Yes. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:122: accelerometer_.sensor->Suspend(); On 2017/06/08 20:26:51, timvolodine wrote: > On 2017/06/07 23:05:07, juncai wrote: > > On 2017/06/07 20:37:35, timvolodine wrote: > > > What happens when we close a window, will that stop the sensors by breaking > > the > > > pipe? Does this also mean that if I unregister all 'ondevicemotion' handlers > > the > > > sensors will be in suspended state? (does this result in some resources > being > > > held?) > > > > When a window is closed, the pipe is closed and the sensors are stopped. If > all > > device motion event listener are unregistered, the sensors will be in a > > suspended state. > > > > Related comments about using Resume() and Suspend() by reillyg@ is at: > > > https://codereview.chromium.org/2896583005/diff/160001/content/renderer/devic... > > I see, so it's optimized for visibility change. Perhaps add a comment to explain > this (something similar to what Reilly said). Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:150: // method doesn't need to be implemented. On 2017/06/08 20:26:51, timvolodine wrote: > On 2017/06/07 23:05:07, juncai wrote: > > On 2017/06/07 20:37:36, timvolodine wrote: > > > sorry if I asked this previously, can we put NOTREACHED() here? (since I > > assume > > > we read the buffer ourselves and don't need this signal) > > > > I don't think NOREACHED() can be put here. > > > > This question was asked and answered here: > > > https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... > > > > I copied the answer below: > > I don't think a DCHECK() or NOTREACHED() can be used here, since this method > is > > called if a sensor is on ON_CHANGE mode and our code doesn't set this mode. > The > > sensor mode is a return value of the mojo call GetSensor(): > > > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... > > This seems strange - we would get unnecessary high frequency mojo IPC calls? > maybe we should set the mode to explicitly reflect the shared memory usage (I > thought it was supported in generic_sensors/)? Yes, we would get high frequency mojo IPC calls. I am working on a CL to fix it by adding a flag to the sensor configuration, so that we can set the flag to notify the browser side not to call SensorReadingChanged() function even when the sensor reporting mode is ON_CHANGE. So eventually NOREACHED() can be added here. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:188: device::mojom::SensorConfiguration::kMaxAllowedFrequency; On 2017/06/08 20:26:51, timvolodine wrote: > On 2017/06/07 23:05:07, juncai wrote: > > On 2017/06/07 20:37:35, timvolodine wrote: > > > nit: probably no need for constexpr? > > > > Done. > > nit: > I actually meant putting > device::mojom::SensorConfiguration::kMaxAllowedFrequency directly inside the > DCHECK_GE (if it's not used anywhere else) I see. Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.h:95: enum class PumpState { STOPPED, RUNNING, PENDING_START }; On 2017/06/08 20:26:51, timvolodine wrote: > On 2017/06/07 23:05:07, juncai wrote: > > On 2017/06/07 20:37:36, timvolodine wrote: > > > This is functionality from the device_sensor_event_pump base class > originally > > > (with Start and Stop methods already implemented there). Can > > > DeviceSensorEventPump be re-used here? > > > > Yes, it will be re-used eventually when the device orientation is refactored. > > Similar to the previously closed bigger CL: > > > https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... > > > > For now, I think it makes sense to have some duplicate code here. It makes > this > > CL cleaner by not modifying any orientation related code since this CL makes > > DeviceMotionEventPump and DeviceOrientationEventPump not share > > DeviceSensorEventPump code. > > I though this CL is only about motion? I was wondering if it's possible to reuse > without touching base class or orientation.. This CL is only about motion. Based on what I see at the current device_sensor_event_pump.h: https://cs.chromium.org/chromium/src/content/renderer/device_sensors/device_s... These functions can not be re-used. The duplication here will eventually be removed when the device orientation is refactored, so I think maybe it is not worth to reuse the current device_sensor_event_pump.h since it will make device_motion_event_pump.h more complicated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.h:95: enum class PumpState { STOPPED, RUNNING, PENDING_START }; On 2017/06/08 23:29:43, juncai wrote: > On 2017/06/08 20:26:51, timvolodine wrote: > > On 2017/06/07 23:05:07, juncai wrote: > > > On 2017/06/07 20:37:36, timvolodine wrote: > > > > This is functionality from the device_sensor_event_pump base class > > originally > > > > (with Start and Stop methods already implemented there). Can > > > > DeviceSensorEventPump be re-used here? > > > > > > Yes, it will be re-used eventually when the device orientation is > refactored. > > > Similar to the previously closed bigger CL: > > > > > > https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... > > > > > > For now, I think it makes sense to have some duplicate code here. It makes > > this > > > CL cleaner by not modifying any orientation related code since this CL makes > > > DeviceMotionEventPump and DeviceOrientationEventPump not share > > > DeviceSensorEventPump code. > > > > I though this CL is only about motion? I was wondering if it's possible to > reuse > > without touching base class or orientation.. > > This CL is only about motion. Based on what I see at the current > device_sensor_event_pump.h: > https://cs.chromium.org/chromium/src/content/renderer/device_sensors/device_s... > These functions can not be re-used. > The duplication here will eventually be removed when the device orientation is > refactored, so I think maybe it is not worth to reuse the current > device_sensor_event_pump.h since it will make device_motion_event_pump.h more > complicated. I guess you could simply keep inheriting from DeviceSensorEventPump and overriding DidStart (making it virtual).. but if you prefer to refactor later e.g. when DeviceOrientation comes along fine with me. Please add a TODO though..
On 2017/06/08 22:53:22, juncai wrote: > On 2017/06/08 20:26:51, timvolodine wrote: > > also, looking at generic_sensors they still seem to be experimental, landing > > this patch would switch motion (which is stable) to the experimental > > implementation (at least partially) - any concerns? > > There are some advantages of switching the sensor backend to > //device/generic_sensor: > 1. //device/generic_sensor/ has better defined mojo interfaces which are more > flexible. > 2. //device/generic_sensor/ has all the sensor types that Chrome needs instead > of spreading the sensor implementation into two places. > 3. From device servicification point of view, //device/generic_sensor is very > close to be servicified, and if we switch the device motion/orientation backend > to //device/generic_sensor, then //device/sensors/ doesn't need to be > servicified since it will be removed. > > There is disadvantage of this as you said, the generic_sensor is still > experimental. But I think the advantage seems much larger than the disadvantage. Sure there are benefits :). My point is that switching motion to generic_sensor/ is semi-equivalent to enabling generic_sensor/ in stable. If there is a reason why generic_sensor/ is still in experimental this may result in crashes or other issues..
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/09 18:55:48, timvolodine wrote: > Sure there are benefits :). My point is that switching motion to generic_sensor/ > is semi-equivalent to enabling generic_sensor/ in stable. If there is a reason > why generic_sensor/ is still in experimental this may result in crashes or other > issues.. The Generic Sensor web APIs are experimental, the rest is just new code. So as long as we're careful we don't have a reason not to use //device/generic_sensor code. To be careful, we have browser integration tests (OS to HTML) and long canary time until next branch point (Release 61 branch point is on July 20th, 2017). So I think it is worth and safe to switch device motion to use //device/generic_sensor/.
https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:150: // method doesn't need to be implemented. On 2017/06/08 23:29:43, juncai wrote: > On 2017/06/08 20:26:51, timvolodine wrote: > > On 2017/06/07 23:05:07, juncai wrote: > > > On 2017/06/07 20:37:36, timvolodine wrote: > > > > sorry if I asked this previously, can we put NOTREACHED() here? (since I > > > assume > > > > we read the buffer ourselves and don't need this signal) > > > > > > I don't think NOREACHED() can be put here. > > > > > > This question was asked and answered here: > > > > > > https://codereview.chromium.org/2896583005/diff/60001/content/renderer/device... > > > > > > I copied the answer below: > > > I don't think a DCHECK() or NOTREACHED() can be used here, since this method > > is > > > called if a sensor is on ON_CHANGE mode and our code doesn't set this mode. > > The > > > sensor mode is a return value of the mojo call GetSensor(): > > > > > > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... > > > > This seems strange - we would get unnecessary high frequency mojo IPC calls? > > maybe we should set the mode to explicitly reflect the shared memory usage (I > > thought it was supported in generic_sensors/)? > > Yes, we would get high frequency mojo IPC calls. I am working on a CL to fix it > by adding a flag to the sensor configuration, so that we can set the flag to > notify the browser side not to call SensorReadingChanged() function even when > the sensor reporting mode is ON_CHANGE. So eventually NOREACHED() can be added > here. Since the CL: https://codereview.chromium.org/2927263002 was landed. NOTREACHED() can be added here. Done. https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.h (right): https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.h:95: enum class PumpState { STOPPED, RUNNING, PENDING_START }; On 2017/06/09 18:54:51, timvolodine wrote: > On 2017/06/08 23:29:43, juncai wrote: > > On 2017/06/08 20:26:51, timvolodine wrote: > > > On 2017/06/07 23:05:07, juncai wrote: > > > > On 2017/06/07 20:37:36, timvolodine wrote: > > > > > This is functionality from the device_sensor_event_pump base class > > > originally > > > > > (with Start and Stop methods already implemented there). Can > > > > > DeviceSensorEventPump be re-used here? > > > > > > > > Yes, it will be re-used eventually when the device orientation is > > refactored. > > > > Similar to the previously closed bigger CL: > > > > > > > > > > https://codereview.chromium.org/2885203004/diff/80001/content/renderer/device... > > > > > > > > For now, I think it makes sense to have some duplicate code here. It makes > > > this > > > > CL cleaner by not modifying any orientation related code since this CL > makes > > > > DeviceMotionEventPump and DeviceOrientationEventPump not share > > > > DeviceSensorEventPump code. > > > > > > I though this CL is only about motion? I was wondering if it's possible to > > reuse > > > without touching base class or orientation.. > > > > This CL is only about motion. Based on what I see at the current > > device_sensor_event_pump.h: > > > https://cs.chromium.org/chromium/src/content/renderer/device_sensors/device_s... > > These functions can not be re-used. > > The duplication here will eventually be removed when the device orientation is > > refactored, so I think maybe it is not worth to reuse the current > > device_sensor_event_pump.h since it will make device_motion_event_pump.h more > > complicated. > > I guess you could simply keep inheriting from DeviceSensorEventPump and > overriding DidStart (making it virtual).. but if you prefer to refactor later > e.g. when DeviceOrientation comes along fine with me. Please add a TODO though.. Added TODO. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/13 23:01:12, juncai wrote: > On 2017/06/09 18:55:48, timvolodine wrote: > > Sure there are benefits :). My point is that switching motion to > generic_sensor/ > > is semi-equivalent to enabling generic_sensor/ in stable. If there is a reason > > why generic_sensor/ is still in experimental this may result in crashes or > other > > issues.. > > The Generic Sensor web APIs are experimental, the rest is just new code. So as > long as we're careful we don't have a reason not to use //device/generic_sensor > code. To be careful, we have browser integration tests (OS to HTML) and long > canary time until next branch point (Release 61 branch point is on July 20th, > 2017). So I think it is worth and safe to switch device motion to use > //device/generic_sensor/. Are there any reasons why generic_sensor APIs are not yet in stable? Any outstanding bugs/crashes/known issues? Maybe launching generic_sensors/ first would make the process a bit smoother..
On 2017/06/14 18:18:03, timvolodine wrote: > Are there any reasons why generic_sensor APIs are not yet in stable? Any > outstanding bugs/crashes/known issues? Maybe launching generic_sensors/ first > would make the process a bit smoother.. To launch a new Web API, steps in the following link need to be taken: https://www.chromium.org/blink/launching-features and Generic Sensor Web APIs are in the middle of this launch process and it takes time, that's why they are not yet in stable. But this doesn't mean the //device/generic_sensor code can not be used in other places, since //device/generic_sensor code is not in experimental. Regarding bugs, here is the open issues of sensors (including device orientation/motion): https://bugs.chromium.org/p/chromium/issues/list?q=component%3ABlink%3ESensor... The outstanding issues are mostly adding new feature. I haven't noticed outstanding crashes from //device/generic_sensor.
To clarify, my previous response is the restructured as following: > Are there any reasons why generic_sensor APIs are not yet in stable? Regarding generic sensor Web APIs not yet being stable, there are two points: 1. The web facing bindings of generic sensor need to follow the launch process of web APIs. To launch a new Web API, steps in the following link need to be taken: https://www.chromium.org/blink/launching-features and Generic Sensor Web APIs are in the middle of this launch process and it takes time, that's why they are not yet in stable. 2. But this doesn't mean the //device/generic_sensor code can not be used in other places, since //device/generic_sensor code is not in experimental, it is just new code. Since it is new code, there is risk. So to be careful, we have browser integration tests (OS to HTML) and long canary time until next branch point (Release 61 branch point is on July 20th, 2017). So I think it is worth and safe to switch device motion/orientation to use //device/generic_sensor/. > Any outstanding bugs/crashes/known issues? Regarding bugs, The outstanding issues are mostly adding new features. Here is the open issues of sensors (including device motion/orientation): https://bugs.chromium.org/p/chromium/issues/list?q=component%3ABlink%3ESensor... There aren't known crashes from //device/generic_sensor. > Maybe launching generic_sensors/ first would make the process a bit smoother.. Waiting for generic sensor Web APIs to be launched will take a long time, and use a larger amount of new code of //device/generic_sensor. Refactoring to use just enough code from //device/generic_sensor for device motion/orientation is ready now.
ping timvolodine@.
On 2017/06/15 01:37:52, juncai wrote: > > Maybe launching generic_sensors/ first would make the process a bit smoother.. > Waiting for generic sensor Web APIs to be launched will take a long time, and > use a larger amount of new code of //device/generic_sensor. Refactoring to use > just enough code from //device/generic_sensor for device motion/orientation is > ready now. Device API team is responsible for the long term maintenance of Device Orientation and the emerging Generic Sensors. I acknowledge that refactoring code introduces risk to shipped APIs. juncai has described how we're mitigating those risks. As a team (juncai, reillyg, scheib) we've decided strategically that refactoring Device Orientation over to the device/generic_sensor backend is the right move at this time, it's a subjective call. Are there remaining concerns that we haven't considered?
scheib@chromium.org changed reviewers: + scheib@chromium.org
On 2017/06/16 21:10:44, scheib - Prefer gerrit tool wrote: > On 2017/06/15 01:37:52, juncai wrote: > > > Maybe launching generic_sensors/ first would make the process a bit > smoother.. > > Waiting for generic sensor Web APIs to be launched will take a long time, and > > use a larger amount of new code of //device/generic_sensor. Refactoring to use > > just enough code from //device/generic_sensor for device motion/orientation is > > ready now. > > Device API team is responsible for the long term maintenance of Device > Orientation and the emerging Generic Sensors. I acknowledge that refactoring > code introduces risk to shipped APIs. juncai has described how we're mitigating > those risks. As a team (juncai, reillyg, scheib) we've decided strategically > that refactoring Device Orientation over to the device/generic_sensor backend is > the right move at this time, it's a subjective call. > > Are there remaining concerns that we haven't considered? OK, don't think I have any other concerns on this issue.. Thanks for all the work juncai@! Hopefully you can keep an eye on any potential issues/crashes while in canary/beta. lgtm
lgtm
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/2896583005/#ps240001 (title: "set suppress_on_change_events_ to true")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497984863449110,
"parent_rev": "d13a0f6d7d1fee7ab355ca1b8d3abc2b8acf6978", "commit_rev":
"687e6a76c9d634404eb93643ab900596a77958fd"}
Message was sent while issue was closed.
Description was changed from ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 ========== to ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2948253002/ by juncai@chromium.org. The reason for reverting is: Need to figure out some issues related to this CL on Android and Mac..
Message was sent while issue was closed.
Description was changed from ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... ========== to ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... ==========
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, jam@chromium.org, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2896583005/#ps260001 (title: "fix issues on Android, Mac and Windows")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498185132546420,
"parent_rev": "a50b4ecbbf6f9b304fef48c6afc7327999a13d02", "commit_rev":
"33f72bfe8b9364e82990f92befa041a08d465d3c"}
Message was sent while issue was closed.
Description was changed from ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... ========== to ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Original-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#481791} Committed: https://chromium.googlesource.com/chromium/src/+/33f72bfe8b9364e82990f92befa0... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/33f72bfe8b9364e82990f92befa0...
Message was sent while issue was closed.
Please add the tests I asked for we discussed this issue offline. This change should probably not have been relanded without another review. https://codereview.chromium.org/2896583005/diff/260001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/260001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:227: if (!sensor || !shared_buffer) This additional condition was left over from your earlier investigations. shared_buffer should never be null when this method is called. This can be a DCHECK.
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2953263002/ by juncai@chromium.org. The reason for reverting is: Need to figure out some issues related to this CL on Windows. To reland, need to add tests on Android and Mac, and fix the issue on Windows..
Message was sent while issue was closed.
Description was changed from ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Original-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#481791} Committed: https://chromium.googlesource.com/chromium/src/+/33f72bfe8b9364e82990f92befa0... ========== to ========== Refactor DeviceMotionEventPump to use //device/generic_sensor instead of //device/sensors //device/generic_sensors already has all the sensors that can be used to implement the DeviceOrientation event: https://w3c.github.io/deviceorientation/spec-source-orientation.html Currently, //content/renderer/device_sensors uses sensors from //device/sensors as its backend, and this is one of the CLs that refactor //content/renderer/device_sensors to use sensors from //device/generic_sensor as the backend and removes //device/sensors. This CL refactors DeviceMotionEvent to use sensors from //device/generic_sensor. The issue page contains the design doc link for this change. BUG=721427 Review-Url: https://codereview.chromium.org/2896583005 Cr-Original-Commit-Position: refs/heads/master@{#480934} Committed: https://chromium.googlesource.com/chromium/src/+/687e6a76c9d634404eb93643ab90... Review-Url: https://codereview.chromium.org/2896583005 Cr-Commit-Position: refs/heads/master@{#481791} Committed: https://chromium.googlesource.com/chromium/src/+/33f72bfe8b9364e82990f92befa0... ==========
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reillyg@, timvolodine@: I updated this CL to fix issues on Android, Mac, and Windows. Please take a look again. Thanks a lot! https://codereview.chromium.org/2896583005/diff/260001/content/renderer/devic... File content/renderer/device_sensors/device_motion_event_pump.cc (right): https://codereview.chromium.org/2896583005/diff/260001/content/renderer/devic... content/renderer/device_sensors/device_motion_event_pump.cc:227: if (!sensor || !shared_buffer) On 2017/06/23 06:45:55, Reilly Grant (use Gerrit) wrote: > This additional condition was left over from your earlier investigations. > shared_buffer should never be null when this method is called. This can be a > DCHECK. Done.
This CL will be closed in favor of two separate CLs: https://chromium-review.googlesource.com/c/550538/ https://chromium-review.googlesource.com/c/550921/ |
