Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2847253002:
[Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor (Closed)
|
Created:
3 years, 7 months ago by juncai Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, shalamov, chromium-reviews, darin (slow to review), Mikhail, mlamouri+watch-sensors_chromium.org, qsr+mojo_chromium.org, riju_, timvolodine, viettrungluu+watch_chromium.org, wanming.lin, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor
This is one of the CLs that make //content/renderer/device_sensors
depend on //device/generic_sensor/public instead of
//device/sensors/public. In order to do that, //device/generic_sensor
needs to support RELATIVE_ORIENTATION sensor type since it is used by
device orientation event.
This CL adds RELATIVE_ORIENTATION sensor implementation on Android to
//device/generic_sensor.
BUG=721427
Review-Url: https://codereview.chromium.org/2847253002
Cr-Commit-Position: refs/heads/master@{#471533}
Committed: https://chromium.googlesource.com/chromium/src/+/52b6764cf200aea49a5c09e221eb612394aabba7
Patch Set 1 : add RELATIVE_ORIENTATION sensor type to //device/generic_sensor on Android #Patch Set 2 : Mac #Patch Set 3 : address comments, added relative orientation sensor on Win, added documentation #Patch Set 4 : updated generic_sensor_consts.h #
Total comments: 2
Patch Set 5 : added tests for new sensors #
Total comments: 8
Patch Set 6 : address nits #
Total comments: 11
Patch Set 7 : rebase #Patch Set 8 : fix typo #
Messages
Total messages: 81 (49 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...
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 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.
Description was changed from ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor BUG=612322 ========== to ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, on ChromeOS, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER, which is already supported in //device/generic_sensor, and on Windows, the RELATIVE_ORIENTATION has the same implementation as ABSOLUTE_ORIENTATION, which is also already supported in //device/generic_sensor. So for these two platforms, RELATIVE_ORIENTATION can be implemented by using the existing sensors in //device/generic_sensor. This CL adds RELATIVE_ORIENTATION sensor implementation on Android and Mac to //device/generic_sensor. BUG=612322 ==========
Description was changed from ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, on ChromeOS, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER, which is already supported in //device/generic_sensor, and on Windows, the RELATIVE_ORIENTATION has the same implementation as ABSOLUTE_ORIENTATION, which is also already supported in //device/generic_sensor. So for these two platforms, RELATIVE_ORIENTATION can be implemented by using the existing sensors in //device/generic_sensor. This CL adds RELATIVE_ORIENTATION sensor implementation on Android and Mac to //device/generic_sensor. BUG=612322 ========== to ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, on ChromeOS, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER, which is already supported in //device/generic_sensor, and on Windows, the RELATIVE_ORIENTATION has the same implementation as ABSOLUTE_ORIENTATION, which is also already supported in //device/generic_sensor. So for these two platforms, RELATIVE_ORIENTATION can be implemented by using the existing sensors in //device/generic_sensor. This CL adds RELATIVE_ORIENTATION sensor implementation on Android and Mac to //device/generic_sensor. BUG=612322 ==========
juncai@chromium.org changed reviewers: + reillyg@chromium.org
Please take a look.
When looking at implementing SensorType::ACCELEROMETER we considered using the Sudden Motion Sensor on macOS however this hardware is deprecated (modern hardware does not have the magnetic hard drives that necessitated including this hardware) so we decided not to bother porting that to the new sensor framework. Given that we are going to be using the generic sensor framework for the older API where we currently have support for it I think it makes sense to add that hardware support as you have done here. However, as currently implemented the RELATIVE_ORIENTATION sensor on Android outputs a quaternion while the macOS implementation outputs the Euler angles that are exposed to the web. We should make this consistent and I would prefer the quaternion representation as it does not suffer from gimbal lock. This sensor can also be implemented on Windows (again, outputting a quaternion) using GUID_SensorType_RelativeOrientation. We can put the conversion from the rotation vector to angles (currently implemented in DeviceSensors.convertRotationVectorToAngles) in the render process when using this sensor to generate DeviceOrientationEvents.
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...
Description was changed from ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, on ChromeOS, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER, which is already supported in //device/generic_sensor, and on Windows, the RELATIVE_ORIENTATION has the same implementation as ABSOLUTE_ORIENTATION, which is also already supported in //device/generic_sensor. So for these two platforms, RELATIVE_ORIENTATION can be implemented by using the existing sensors in //device/generic_sensor. This CL adds RELATIVE_ORIENTATION sensor implementation on Android and Mac to //device/generic_sensor. BUG=612322 ========== to ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/01 18:22:05, Reilly Grant wrote: > When looking at implementing SensorType::ACCELEROMETER we considered using the > Sudden Motion Sensor on macOS however this hardware is deprecated (modern > hardware does not have the magnetic hard drives that necessitated including this > hardware) so we decided not to bother porting that to the new sensor framework. > > Given that we are going to be using the generic sensor framework for the older > API where we currently have support for it I think it makes sense to add that > hardware support as you have done here. However, as currently implemented the > RELATIVE_ORIENTATION sensor on Android outputs a quaternion while the macOS > implementation outputs the Euler angles that are exposed to the web. We should > make this consistent and I would prefer the quaternion representation as it does > not suffer from gimbal lock. > > This sensor can also be implemented on Windows (again, outputting a quaternion) > using GUID_SensorType_RelativeOrientation. > > We can put the conversion from the rotation vector to angles (currently > implemented in DeviceSensors.convertRotationVectorToAngles) in the render > process when using this sensor to generate DeviceOrientationEvents. Done.
alexander.shalamov@intel.com changed reviewers: + alexander.shalamov@intel.com
https://codereview.chromium.org/2847253002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2847253002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:197: // RelativeOrientation sensor reader initialization parameters. Could you please add test for new sensor type?
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/2847253002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2847253002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:197: // RelativeOrientation sensor reader initialization parameters. On 2017/05/02 06:22:49, shalamov wrote: > Could you please add test for new sensor type? Done.
Description was changed from ========== Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ========== to ========== [device service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ==========
Description was changed from ========== [device service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ==========
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...)
lgtm with nits https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_accelerometer_mac.cc:23: DCHECK_EQ(type, mojom::SensorType::ACCELEROMETER); If this is the only valid value remove the |type| argument and pass this constant directly to PlatformSensor(). https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:6: #include <Sensorsdef.h> SensorsDef.h https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_unittest.cc:156: TestSensorCreateCallback callback9; Wrap each of these in its own scope so that you don't have to come up with separate variable names: { TestSensorCreateCallback callback; scoped_refptr<PlatformSensor> sensor = CreateSensor(SensorType::RELATIVE_ORIENTATION, &callback); ASSERT_TRUE(sensor); EXPECT_EQ(SensorType:RELATIVE_ORIENTATION, sensor->GetType()); } https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:8: #include <Sensorsdef.h> SensorsDef.h
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: + dcheng@chromium.org, mark@chromium.org
mark@chromium.org: Please review changes in device/generic_sensor/DEPS that adds '+third_party/sudden_motion_sensor' dcheng@chromium.org: Please do a security review of changes in device/generic_sensor/public/interfaces/sensor.mojom https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_accelerometer_mac.cc:23: DCHECK_EQ(type, mojom::SensorType::ACCELEROMETER); On 2017/05/02 23:23:31, Reilly Grant wrote: > If this is the only valid value remove the |type| argument and pass this > constant directly to PlatformSensor(). Done. https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:6: #include <Sensorsdef.h> On 2017/05/02 23:23:31, Reilly Grant wrote: > SensorsDef.h Done. https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_unittest.cc:156: TestSensorCreateCallback callback9; On 2017/05/02 23:23:31, Reilly Grant wrote: > Wrap each of these in its own scope so that you don't have to come up with > separate variable names: > > { > TestSensorCreateCallback callback; > scoped_refptr<PlatformSensor> sensor = > CreateSensor(SensorType::RELATIVE_ORIENTATION, &callback); > ASSERT_TRUE(sensor); > EXPECT_EQ(SensorType:RELATIVE_ORIENTATION, sensor->GetType()); > } Done. https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2847253002/diff/80001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:8: #include <Sensorsdef.h> On 2017/05/02 23:23:31, Reilly Grant wrote: > SensorsDef.h Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com
https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:28: return mojom::ReportingMode::ON_CHANGE; so, does the accelerometer give updated values only when the device is actually moving? what values does it give when the device is still?
https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:28: return mojom::ReportingMode::ON_CHANGE; On 2017/05/03 07:47:46, Mikhail wrote: > so, does the accelerometer give updated values only when the device is actually > moving? what values does it give when the device is still? Yes. When the device is still, the values are 0 for x, y, and z-axis.
https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:28: return mojom::ReportingMode::ON_CHANGE; On 2017/05/03 07:47:46, Mikhail wrote: > so, does the accelerometer give updated values only when the device is actually > moving? what values does it give when the device is still? The reporting mode for Accelerometer is the same as Ambient Light on Mac. I will have a follow-up CL to only update the blink side when the accelerometer value changes.
Description was changed from ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. TBR=dcheng@chromium.org for security review of sensor.mojom file change. BUG=612322 ==========
Description was changed from ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. TBR=dcheng@chromium.org for security review of sensor.mojom file change. BUG=612322 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. TBR=dcheng@chromium.org for security review of change in //device/generic_sensor/public/interfaces/sensor.mojom BUG=612322 ==========
lgtm
Btw, please don't TBR changes like this, it's not really a non-trivial change since it's exposing more things. https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:35: mojom::SensorConfiguration::kMaxAllowedFrequency; Is 60Hz actually OK? My understanding is there are some practical attacks to inferring passwords at high enough sampling rates from the accelerometer: see http://www.hotmobile.org/2012/papers/HotMobile12-final42.pdf for example. https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:202: base::Bind([](ISensorDataReport& report, SensorReading& reading) { Google style does not permit mutable reference arguments. Please fix this as a followup. https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/public/cpp/sensor_reading.h (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/public/cpp/sensor_reading.h:98: // device in 3D space. Maybe we should be using a union? That seems like it'd much more self documenting and less prone to randomly using the wrong index. Something else to consider as a followup...
Description was changed from ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. TBR=dcheng@chromium.org for security review of change in //device/generic_sensor/public/interfaces/sensor.mojom BUG=612322 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ==========
On 2017/05/03 20:58:53, dcheng wrote: > Btw, please don't TBR changes like this, it's not really a non-trivial change > since it's exposing more things. To be clear: the reason I told juncai@ to TBR you is that I thought this patch was just adding a bunch of clarifying comments to the .mojom file. I forgot that we were also adding a new sensor type to the enum as well. We are, however, not exposing anything new. This sensor type is already part of the //device/sensor Mojo interface which we are replacing with the //device/generic_sensor Mojo interface. The existing //device/sensor code also polls at 60Hz. I think that this is an excellent discussion to have, but not on this review. There is an active discussion about introducing permissions for high-resolution vs. low-resolution sensor data happening on the spec: https://github.com/w3c/sensors/issues/183
On 2017/05/03 21:20:24, Reilly Grant wrote: > On 2017/05/03 20:58:53, dcheng wrote: > > Btw, please don't TBR changes like this, it's not really a non-trivial change > > since it's exposing more things. > > To be clear: the reason I told juncai@ to TBR you is that I thought this patch > was just adding a bunch of clarifying comments to the .mojom file. I forgot that > we were also adding a new sensor type to the enum as well. > > We are, however, not exposing anything new. This sensor type is already part of > the //device/sensor Mojo interface which we are replacing with the > //device/generic_sensor Mojo interface. > > The existing //device/sensor code also polls at 60Hz. I think that this is an > excellent discussion to have, but not on this review. There is an active > discussion about introducing permissions for high-resolution vs. low-resolution > sensor data happening on the spec: https://github.com/w3c/sensors/issues/183 1) Is this web-exposed already? Or is it behind a runtime flag? 2) I don't really understand the difference between sensors and generic_sensors. Is that documented somewhere? I also don't really know where this is moving from, or if where it's moving from is web-exposed. I vaguely recall reviewing some changes mojofying this in the past, but IIRC, the old interface didn't support configuring a frequency, while this one does? 3) (Not caused by this CL, but) it's also really hard to understand what the different configuration constraints are, since they are scattered throughout the code and depend on values that are populated at runtime. Is it possible to improve this?
> The existing //device/sensor code also polls at 60Hz. I think that this is an > excellent discussion to have, but not on this review. There is an active > discussion about introducing permissions for high-resolution vs. low-resolution > sensor data happening on the spec: https://github.com/w3c/sensors/issues/183 I don't find that discussion satisfying for a variety of reasons. I think we should hold off on landing this code until the spec, and security and privacy analysis, are done (which requires use cases tightly linked to the actual sensor granularities). I also don't think "but we already do it" is a good justification to keep doing it, or to encode it in a new place. We're going to need a coherent story for enforcing sensing granularity and permissions/prompts before we ship this/turn the flag on, and the more places we add ad hoc checks and limits, the more work it'll be later to rationalize it. Honestly I didn't know we had even gotten to the implementation phase of Generic Sensor API. It seems early to me.
palmer@google.com changed reviewers: + palmer@chromium.org
On 2017/05/03 22:19:16, Chris Palmer wrote: > > The existing //device/sensor code also polls at 60Hz. I think that this is an > > excellent discussion to have, but not on this review. There is an active > > discussion about introducing permissions for high-resolution vs. > low-resolution > > sensor data happening on the spec: https://github.com/w3c/sensors/issues/183 > > I don't find that discussion satisfying for a variety of reasons. I think we > should hold off on landing this code until the spec, and security and privacy > analysis, are done (which requires use cases tightly linked to the actual sensor > granularities). > > I also don't think "but we already do it" is a good justification to keep doing > it, or to encode it in a new place. We're going to need a coherent story for > enforcing sensing granularity and permissions/prompts before we ship this/turn > the flag on, and the more places we add ad hoc checks and limits, the more work > it'll be later to rationalize it. Honestly I didn't know we had even gotten to > the implementation phase of Generic Sensor API. It seems early to me. Let me back up and try to provide some more context for this change. //device/sensor provides a Mojo service which is used to implement the DeviceOrientation Event Specification (https://www.w3.org/TR/orientation-event/). This is the only part of this that has been shipped and exposes device orientation and motion events to the web at 60Hz. //device/generic_sensors provides a Mojo service which is used to implement the family of sensor specifications under the umbrella of the Generic Sensors API (https://w3c.github.io/sensors/). None of these new APIs have shipped. While there are deficiencies in the design of //device/generic_sensors I believe it is, in general, a better abstraction of the platform than //device/sensor and so the work that juncai@ is doing here is to update the implementation of the DeviceOrientation Event Specification (//third_party/WebKit/Source/modules/device_orientation) to use this new Mojo service. This is a refactoring to simplify the implementation and remove duplicated code. We are not shipping anything new. By consolidating our sensor APIs to use a single Mojo service we will be able to enforce granularity and permissions in a single place.
https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:28: return mojom::ReportingMode::ON_CHANGE; On 2017/05/03 17:39:18, juncai wrote: > On 2017/05/03 07:47:46, Mikhail wrote: > > so, does the accelerometer give updated values only when the device is > actually > > moving? what values does it give when the device is still? > > Yes. When the device is still, the values are 0 for x, y, and z-axis. Accelerometer in rest should provide 9.8 ms2 acceleration (length of vector). If you get all 0s, then it is mojom::SensorType::LINEAR_ACCELERATION sensor.
https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_accelerometer_mac.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:28: return mojom::ReportingMode::ON_CHANGE; On 2017/05/04 06:53:46, shalamov wrote: > On 2017/05/03 17:39:18, juncai wrote: > > On 2017/05/03 07:47:46, Mikhail wrote: > > > so, does the accelerometer give updated values only when the device is > > actually > > > moving? what values does it give when the device is still? > > > > Yes. When the device is still, the values are 0 for x, y, and z-axis. > > Accelerometer in rest should provide 9.8 ms2 acceleration (length of vector). > If you get all 0s, then it is mojom::SensorType::LINEAR_ACCELERATION sensor. Sorry for the confusion. I did some further reading and found that when the device is still, the value on z-axis is 9.8 instead of 0. https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_accelerometer_mac.cc:28: return mojom::ReportingMode::ON_CHANGE; On 2017/05/03 18:13:03, juncai wrote: > On 2017/05/03 07:47:46, Mikhail wrote: > > so, does the accelerometer give updated values only when the device is > actually > > moving? what values does it give when the device is still? > > The reporting mode for Accelerometer is the same as Ambient Light on Mac. I will > have a follow-up CL to only update the blink side when the accelerometer value > changes. Bug filed: https://bugs.chromium.org/p/chromium/issues/detail?id=718460 I will do this in a follow-up CL. https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:202: base::Bind([](ISensorDataReport& report, SensorReading& reading) { On 2017/05/03 20:58:53, dcheng wrote: > Google style does not permit mutable reference arguments. Please fix this as a > followup. Bug filed: https://bugs.chromium.org/p/chromium/issues/detail?id=718457 I will fix it in a follow-up CL. https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... File device/generic_sensor/public/cpp/sensor_reading.h (right): https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/... device/generic_sensor/public/cpp/sensor_reading.h:98: // device in 3D space. On 2017/05/03 20:58:53, dcheng wrote: > Maybe we should be using a union? That seems like it'd much more self > documenting and less prone to randomly using the wrong index. Something else to > consider as a followup... Filed bug: https://bugs.chromium.org/p/chromium/issues/detail?id=718459 I will consider fixing it in a follow-up CL.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2847253002/#ps100001 (title: "address nits")
Oops, sorry, I mis-clicked and clicked the Close button by accident!
This CL does most of the work for enabling https://w3c.github.io/orientation-sensor/#relativeorientationsensor-interface , would be great to see it landed :)
On 2017/05/10 13:06:30, Mikhail wrote: > This CL does most of the work for enabling > https://w3c.github.io/orientation-sensor/#relativeorientationsensor-interface , > would be great to see it landed :) We're trying to figure out the security side of things before proceeding further.
Description was changed from ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=612322 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=721427 ==========
Following this chain of links: https://w3c.github.io/orientation-sensor/#usecases-requirements https://w3c.github.io/motion-sensors/#usecases-and-requirements https://w3c.github.io/motion-sensors/#security-and-privacy https://www.w3.org/TR/generic-sensor/#security-and-privacy I'm left with the impression that we have a sensor that has high sample rate and precision, and that that is necessary to support the use-cases, but that we don't have a clear story on what kind of control/informed consent/visibility people will have when using origins that use this API. In particular, I have a feeling that we are not going to be able to have one generic protection mechanism for all the sensors, because the sensors support use-cases with different requirements. (Consider this API vs. the Ambient Light Sensor API, for example. See https://bugs.chromium.org/p/chromium/issues/detail?id=642731#c17) Before I can give a security thumbs-up, I'd like to see realistic, non-speculative use-cases for all the generic sensor APIs, and safety mechanisms suited to each API/sensor type. The most likely available sensor mechanisms are: * Reducing sample rate to the minimum necessary * Reducing sample precision to the minimum necessary * Requiring callers to be in the currently-focused tab * Requiring callers to be in the top-level document * Requiring callers to have received a user gesture before getting some samples * Requiring callers to have received an origin-scoped permission (prompting) (this is probably the least good defense mechanism, but may sometimes be necessary) The chrome-security-enamel@google.com team may be able to help you nail down which defenses are best when and for which sensors.
On 2017/05/12 00:24:19, palmer wrote: > Following this chain of links: > > https://w3c.github.io/orientation-sensor/#usecases-requirements > > https://w3c.github.io/motion-sensors/#usecases-and-requirements > > https://w3c.github.io/motion-sensors/#security-and-privacy > > https://www.w3.org/TR/generic-sensor/#security-and-privacy > > I'm left with the impression that we have a sensor that has high sample rate and > precision, and that that is necessary to support the use-cases, but that we > don't have a clear story on what kind of control/informed consent/visibility > people will have when using origins that use this API. > > In particular, I have a feeling that we are not going to be able to have one > generic protection mechanism for all the sensors, because the sensors support > use-cases with different requirements. (Consider this API vs. the Ambient Light > Sensor API, for example. See > https://bugs.chromium.org/p/chromium/issues/detail?id=642731#c17) > > Before I can give a security thumbs-up, I'd like to see realistic, > non-speculative use-cases for all the generic sensor APIs, and safety mechanisms > suited to each API/sensor type. The most likely available sensor mechanisms are: > > * Reducing sample rate to the minimum necessary > * Reducing sample precision to the minimum necessary > * Requiring callers to be in the currently-focused tab > * Requiring callers to be in the top-level document > * Requiring callers to have received a user gesture before getting some samples > * Requiring callers to have received an origin-scoped permission (prompting) > (this is probably the least good defense mechanism, but may sometimes be > necessary) > > The mailto:chrome-security-enamel@google.com team may be able to help you nail down > which defenses are best when and for which sensors. So that this refactoring work is not blocked do we agree that the intent of this patch is not to change the status quo for shipping APIs and can be landed before consensus on the Generic Sensors API is reached?
> So that this refactoring work is not blocked do we agree that the intent of this > patch is not to change the status quo for shipping APIs and can be landed before > consensus on the Generic Sensors API is reached? I support that point: This change has raised awareness of an issue (let's revisit sensor precision, frequency, and prompts) but that is work outside the scope of this change.
Okay, IPC security LGTM.
So just to be clear, does this mean the //device/sensors implementation is going away at some point in the future?
On 2017/05/12 21:32:25, dcheng wrote: > So just to be clear, does this mean the //device/sensors implementation is going > away at some point in the future? Yes. I am working on a follow-up CL that removes the //device/sensors directory and let //content/renderer/device_sensors depend on //device/generic_sensor implementation.
ok, lgtm in that case. thanks
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...
Description was changed from ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. At //device/sensors, the RELATIVE_ORIENTATION is implemented using ACCELEROMETER on ChromeOS and Mac, and //device/generic_sensor already implemented ACCELEROMETER on ChromeOS, but not on Mac. So this CL adds ACCELEROMETER sensor implementation to //device/generic_sensor on Mac. This CL also adds RELATIVE_ORIENTATION sensor implementation on Android and Windows to //device/generic_sensor. BUG=721427 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. This CL adds RELATIVE_ORIENTATION sensor implementation on Android to //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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
reillyg@, please take another look. I did some changes as following: 1. Removed RELATIVE_ORIENTATION sensor Windows implementation using GUID_SensorType_RelativeOrientation from Patch Set 6 since it is defined in the sensor device driver reference, but not in the sensor API programming reference. 2. Moved code which adds ACCELEROMETER implementation to //device/generic_sensor on Mac to another CL: https://codereview.chromium.org/2880743003/
lgtm, thanks for breaking this up
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, palmer@chromium.org, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/2847253002/#ps140001 (title: "fix typo")
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": 140001, "attempt_start_ts": 1494636917822330,
"parent_rev": "e6a66337552cfd9a9af9471775ad5b67ee588b88", "commit_rev":
"52b6764cf200aea49a5c09e221eb612394aabba7"}
Message was sent while issue was closed.
Description was changed from ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. This CL adds RELATIVE_ORIENTATION sensor implementation on Android to //device/generic_sensor. BUG=721427 ========== to ========== [Device Service] Add RELATIVE_ORIENTATION sensor type to //device/generic_sensor This is one of the CLs that make //content/renderer/device_sensors depend on //device/generic_sensor/public instead of //device/sensors/public. In order to do that, //device/generic_sensor needs to support RELATIVE_ORIENTATION sensor type since it is used by device orientation event. This CL adds RELATIVE_ORIENTATION sensor implementation on Android to //device/generic_sensor. BUG=721427 Review-Url: https://codereview.chromium.org/2847253002 Cr-Commit-Position: refs/heads/master@{#471533} Committed: https://chromium.googlesource.com/chromium/src/+/52b6764cf200aea49a5c09e221eb... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/52b6764cf200aea49a5c09e221eb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
