|
|
Created:
3 years, 8 months ago by riju_ Modified:
3 years, 4 months ago Reviewers:
benwells, jochen (gone - plz use gerrit), shalamov, mlamouri (slow - plz ping), timvolodine, kinuko, raymes, Tobias Sargeant, Tom Sepez, Reilly Grant (use Gerrit), brettw, owencm, Mikhail CC:
Aaron Boodman, abarth-chromium, shalamov, android-webview-reviews_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, haraken, jam, Mikhail, mlamouri+watch-permissions_chromium.org, mlamouri+watch-blink_chromium.org, qsr+mojo_chromium.org, raymes+watch_chromium.org, Reilly Grant (use Gerrit), viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[sensors][permission] Add new permission types in permission module.
This is the first part of adding permission guard for sensors based on Generic Sensor Framework.
There was consensus to add granular permissions for sensors here
https://github.com/w3c/sensors/issues/22
This CL adds permission name for the different sensors in the permission module.
Spec discussion : https://github.com/w3c/permissions/pull/142
BUG=606766
Review-Url: https://codereview.chromium.org/2791623004
Cr-Commit-Position: refs/heads/master@{#491303}
Committed: https://chromium.googlesource.com/chromium/src/+/7b072ad1ed828f70c9255c9c863542f72d8863b9
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add Layout tests for the newly added permission types #Patch Set 3 : Add runtime flag check for sensor permissions #Patch Set 4 : Remove orientation-sensor permission string. #
Total comments: 2
Patch Set 5 : Fix mounir's comment #Patch Set 6 : Rebase #
Total comments: 3
Patch Set 7 : Fix reilly's comment: 1 PermissionType SENSORS for all sensors. #
Total comments: 4
Patch Set 8 : Use 1 single permissionName SENSORS, add ContentSettings #
Total comments: 6
Patch Set 9 : fixing nits #
Total comments: 2
Patch Set 10 : Fix owners #
Total comments: 3
Patch Set 11 : Rebase + add blank line in ONWERS file. #Messages
Total messages: 167 (103 generated)
The CQ bit was checked by rijubrata.bhaumik@intel.com 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 ========== [sensors][permission] Add Permission guard to the generic sensor apis. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ========== to ========== [sensors][permission] Add new permission stings in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ==========
rijubrata.bhaumik@intel.com changed reviewers: + mlamouri@chromium.org
Description was changed from ========== [sensors][permission] Add new permission stings in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ========== to ========== [sensors][permission] Add new permission strings in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ==========
rijubrata.bhaumik@intel.com changed reviewers: + timvolodine@chromium.org
Mounir : For some context, this is the WIP CL (https://codereview.chromium.org/2458453002/) for some overall picture. I am splitting the WIP CL for easier review. This is the first installment. Adding Tim, as he is aware of our origin trial plan and outcome of privacy reviews.
A comment/question below. Also, do we need all the permission types? I understood from my initial scan of the thread that some of them would be opt-out while others would not require permissions.. https://codereview.chromium.org/2791623004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:331: case PermissionType::ORIENTATION_SENSOR: Is this a good name / should it be here? While the preceding sensors are 'real' physical sensors, the 'orientation sensor' typically uses information from a combination of preceding sensors (depending on what kind of orientation you want to obtain so it may be not well defined here).
On 2017/04/03 16:57:17, timvolodine wrote: > A comment/question below. Also, do we need all the permission types? I > understood from my initial scan of the thread that some of them would be opt-out > while others would not require permissions.. > Something agreed upon https://github.com/w3c/sensors/issues/22#issuecomment-285663269 Right now, the chrome privacy team suggested : AMBIENT_LIGHT_SENSOR: opt-out UI. granted by default. MAGNETOMETER: opt-out UI. granted by default. ORIENTATION_SENSOR: opt-out UI. granted by default. (To be confirmed) ACCELEROMETER: auto-grant. no UI. GYROSCOPE: auto grant. no UI. something i put up in a subsequent patch (https://codereview.chromium.org/2789053002) Presently I am treating accelerometer,gyroscope as constant permission (https://codereview.chromium.org/2788483006) If I don't use permission strings for accelerometer/gyroscope, then navigator.query() for these 2 will be invalid. For ALS and magnetometer, navigator.query would be valid. So to remove this inconsistency, all the permission strings (granular) are necessary. All concrete sensor specs also define their permissions strings tied to the permission module. The privacy policy is left to the browser/implementer, something possible because of granular permission strings. > https://codereview.chromium.org/2791623004/diff/1/android_webview/browser/aw_... > File android_webview/browser/aw_permission_manager.cc (right): > > https://codereview.chromium.org/2791623004/diff/1/android_webview/browser/aw_... > android_webview/browser/aw_permission_manager.cc:331: case > PermissionType::ORIENTATION_SENSOR: > Is this a good name / should it be here? While the preceding sensors are 'real' > physical sensors, the 'orientation sensor' typically uses information from a > combination of preceding sensors (depending on what kind of orientation you want > to obtain so it may be not well defined here). This question came up for debate. As of now, we can think of permission on interface ("physical" sensor + "fused" sensor), instead of just physical sensor. Let me CC some chrome privacy/security people to voice their opinions, and also assert if we got the right message.
rijubrata.bhaumik@intel.com changed reviewers: + benwells@chromium.org, raymes@chromium.org
Adding @raymes, @benwells from the chrome privacy team, to make sure we are all in the same page.
A couple of comments: - I think the description of this CL is misleading. Can you change to 'Add new permission types...' (permission strings sounds to me like the strings we use in the UI). - I have the same question about orientation_sensor, and don't really understand the answer given in a previous comment
Description was changed from ========== [sensors][permission] Add new permission strings in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ========== to ========== [sensors][permission] Add new permission types in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ==========
Thanks for the comments ! On 2017/04/04 00:49:37, benwells wrote: > A couple of comments: > - I think the description of this CL is misleading. Can you change to 'Add new > permission types...' (permission strings sounds to me like the strings we use in > the UI). Done. > - I have the same question about orientation_sensor, and don't really understand > the answer given in a previous comment We feel that 'orientation-sensor' permission type is necessary mainly because : 1. OrientationSensor interface provides device orientation data which is not the same as the data provided by the low level sensors (magnetic field, angular velocity), even though low-level sensors can be used internally to obtain orientation data. Our understanding is that the 'orientation-sensor' permission is granting the access to the data (i.e. the exposed by the AbsoluteOrientationSensor interface), not to the physical sensors of the device. 2. Forward compatibility: It will help to make changes in privacy policy at a granular level. Later on UA(chrome) can decide to restrict access to say gyroscope, but think that orientation-sensor is fine. (although gyroscope hardware is used for orientation-sensor data) Use case : some privacy leaks happen only from gyro data, and its is sufficiently hard to reconstruct gyro data from orientation-sensor data.
Should LayoutTests be added?
The CQ bit was checked by rijubrata.bhaumik@intel.com 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/04/04 12:54:26, mlamouri wrote: > Should LayoutTests be added? I added some basic tests now. More tests will be added in https://codereview.chromium.org/2797713002 after https://codereview.chromium.org/2789053002 https://codereview.chromium.org/2788483006
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sgtm but isn't this exposing the feature to the web?
On 2017/04/05 13:17:22, mlamouri wrote: > sgtm but isn't this exposing the feature to the web? Sensors are behind the kGenericSensor flag. This CL does not expose anything extra. Does it answer your question?
On 2017/04/05 at 13:33:01, rijubrata.bhaumik wrote: > On 2017/04/05 13:17:22, mlamouri wrote: > > sgtm but isn't this exposing the feature to the web? > > Sensors are behind the kGenericSensor flag. > This CL does not expose anything extra. > > Does it answer your question? Doesn't it allow someone to call `navigator.permissions.query({name: '...'})`?
On 2017/04/05 16:11:31, mlamouri wrote: > On 2017/04/05 at 13:33:01, rijubrata.bhaumik wrote: > > On 2017/04/05 13:17:22, mlamouri wrote: > > > sgtm but isn't this exposing the feature to the web? > > > > Sensors are behind the kGenericSensor flag. > > This CL does not expose anything extra. > > > > Does it answer your question? > > Doesn't it allow someone to call `navigator.permissions.query({name: '...'})`? Yes. it does.
On 2017/04/05 18:01:37, riju_ wrote: > On 2017/04/05 16:11:31, mlamouri wrote: > > On 2017/04/05 at 13:33:01, rijubrata.bhaumik wrote: > > > On 2017/04/05 13:17:22, mlamouri wrote: > > > > sgtm but isn't this exposing the feature to the web? > > > > > > Sensors are behind the kGenericSensor flag. > > > This CL does not expose anything extra. > > > > > > Does it answer your question? > > > > Doesn't it allow someone to call `navigator.permissions.query({name: '...'})`? > > Yes. it does. as shown in the layout test at LayoutTests/http/tests/permissions/resources/test-query.js
rijubrata.bhaumik@intel.com changed reviewers: + tsepez@chromium.org
tsepez@ : for permission.mojom file.
lgtm
@raymes: Can you take a look please.
Hey riju, I'm not sure if benwells/timvolodine questions have been addressed sufficiently. Could you add a section to your design doc about what you plan for the web exposed permissions to be so that consensus can be reached there? My personal opinion is that it complicates things here to have an extra permission for the orientation API because it's not mutually exclusive with gyroscope, accelerometer, etc. It would mean that we have to define the interactions between those permissions clearly. If we could instead have permissions for the most granular types of data (e.g. accelerometer data, gyroscope data, etc.) then any APIs built on top of those data sources can return results according to the permissions that are granted. Thanks!
On 2017/04/06 05:32:55, raymes wrote: > Hey riju, I'm not sure if benwells/timvolodine questions have been addressed > sufficiently. Could you add a section to your design doc about what you plan for > the web exposed permissions to be so that consensus can be reached there? > > My personal opinion is that it complicates things here to have an extra > permission for the orientation API because it's not mutually exclusive with > gyroscope, accelerometer, etc. It would mean that we have to define the > interactions between those permissions clearly. If we could instead have > permissions for the most granular types of data (e.g. accelerometer data, > gyroscope data, etc.) then any APIs built on top of those data sources can > return results according to the permissions that are granted. > > Thanks! @raymes thanks for comments! We added more information to Security & Privacy section of design document https://docs.google.com/document/d/1Ml65ZdW5AgIsZTszk4mD_ohr40pcrdVFOIf0ZtWxDv0 From security perspective, data provided by OrientationSensor exposes much less user sensitive information comparing to low-level data that is used for fusion. In my opinion, it is impossible to reconstruct original data provided by accelerometer, gyroscope or magnetometer from quaternion or rotation matrix. We think it would be better to enforce stricter security policies for low-level sensors and maybe relax policies for highly processed (fused) data. @mlamouri Do you have any ideas how to put added enum values under the flag? Strings are not exposed from the blink, but Permission API behavior would be changed, e.g., query(Dict) method would not throw anymore.
On 2017/04/07 at 07:37:30, alexander.shalamov wrote: > @mlamouri > > Do you have any ideas how to put added enum values under the flag? > Strings are not exposed from the blink, but Permission API behavior > would be changed, e.g., query(Dict) method would not throw anymore. In Permissions.cpp, you are adding support to create the right Mojo object for the permissions. In this method, the permission dictionary is parsed. You should probably add a RuntimeEnabledFeature that, when not enabled, will throw when one of these permissions is found. The change will be somewhat web visible because the exception wouldn't exactly be the same but I think it should be fine.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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.
mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com
raymes@, we have updated the design doc with the permissions considerations (https://docs.google.com/document/d/1Ml65ZdW5AgIsZTszk4mD_ohr40pcrdVFOIf0ZtWxDv0). After we got some more feedback and references to research papers we updated the sensors permission model so that the AbsoluteOrientationSensor does not have a dedicated security token and instead requires a combination of the tokens for the underlying low-level sensors ("accelerometer", "gyroscope", "magnetometer"). We also added proposals for opt-out permissions UI. Could you please take a look and share your opinion?
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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.
@raymes: PR for the permission spec for this corresponding change has now landed. (https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1...) We have updated the design doc also. We would like to know if there are any more blockers for this CL.
raymes@chromium.org changed reviewers: + owencm@chromium.org
+owencm who is PM of this feature for Chrome On 2017/04/19 10:20:22, riju_ wrote: > @raymes: PR for the permission spec for this corresponding change has now > landed. > (https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1...) > > We have updated the design doc also. > We would like to know if there are any more blockers for this CL. Thanks for your patience! This generally looks good to me. Unfortunately there is still some ongoing discussion (that I only just became aware of) of the privacy requirements for Chrome. Although it's unlikely this part of the CL will change, I'd rather wait until those things are resolved before we land this code if possible. We definitely won't be able to land subsequent CLs until those things are resolved anyway. AFAIU owencm@ is the point of contact for status.
Expanding a little, there's a reasonable chance Chrome may want to explicitly differentiate between a "high accuracy" and "low accuracy" mode for these sensors in terms of our permissions UI, but further it's not entirely clear exactly how this will work. Do we have a sense for how the permissions API could support those kind of options by the way? In general I'd suggest holding off right now unless it's clear that the above high/low accuracy model could be supported with this update to the spec since otherwise we may need to change to support Chrome's privacy/security model. On Wed, Apr 19, 2017 at 8:20 PM <raymes@chromium.org> wrote: > +owencm who is PM of this feature for Chrome > > On 2017/04/19 10:20:22, riju_ wrote: > > @raymes: PR for the permission spec for this corresponding change has now > > landed. > > > ( > https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1... > ) > > > > We have updated the design doc also. > > We would like to know if there are any more blockers for this CL. > > Thanks for your patience! This generally looks good to me. Unfortunately > there > is still some ongoing discussion (that I only just became aware of) of the > privacy requirements for Chrome. Although it's unlikely this part of the > CL will > change, I'd rather wait until those things are resolved before we land > this code > if possible. We definitely won't be able to land subsequent CLs until those > things are resolved anyway. > > AFAIU owencm@ is the point of contact for status. > > https://codereview.chromium.org/2791623004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Expanding a little, there's a reasonable chance Chrome may want to explicitly differentiate between a "high accuracy" and "low accuracy" mode for these sensors in terms of our permissions UI, but further it's not entirely clear exactly how this will work. Do we have a sense for how the permissions API could support those kind of options by the way? In general I'd suggest holding off right now unless it's clear that the above high/low accuracy model could be supported with this update to the spec since otherwise we may need to change to support Chrome's privacy/security model. On Wed, Apr 19, 2017 at 8:20 PM <raymes@chromium.org> wrote: > +owencm who is PM of this feature for Chrome > > On 2017/04/19 10:20:22, riju_ wrote: > > @raymes: PR for the permission spec for this corresponding change has now > > landed. > > > ( > https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1... > ) > > > > We have updated the design doc also. > > We would like to know if there are any more blockers for this CL. > > Thanks for your patience! This generally looks good to me. Unfortunately > there > is still some ongoing discussion (that I only just became aware of) of the > privacy requirements for Chrome. Although it's unlikely this part of the > CL will > change, I'd rather wait until those things are resolved before we land > this code > if possible. We definitely won't be able to land subsequent CLs until those > things are resolved anyway. > > AFAIU owencm@ is the point of contact for status. > > https://codereview.chromium.org/2791623004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm assuming raymes@ is happy with it https://codereview.chromium.org/2791623004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right): https://codereview.chromium.org/2791623004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/permissions/Permissions.cpp:106: IsSensorRuntimeFlagEnabled(exception_state)) Instead of checking for the flag N times, could you do: ``` if (name == "ambient-ligth-sensor" || name == "accelerometer" || name = "gyroscope" || name == "magnetometer") { if (!RuntimeEnabledFeatures::sensorEnabled()) { exception_state.ThrowTypeError("GenericSensor flag is not enabled."); return nullptr; } // check for all permissions } ```
The CQ bit was checked by rijubrata.bhaumik@intel.com 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.
@raymes, @owen: After getting to know the concerns from Privacy team, regarding low and high resolution modes, we created a Github issue (https://github.com/w3c/sensors/issues/183) summarizing what it might mean. The opinion of our team is listed here: https://github.com/w3c/sensors/issues/183#issuecomment-297341213 We would like some input from the Chrome privacy team regarding this, in the GH issue.
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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.
On 2017/04/20 03:20:52, raymes wrote: > +owencm who is PM of this feature for Chrome > > On 2017/04/19 10:20:22, riju_ wrote: > > @raymes: PR for the permission spec for this corresponding change has now > > landed. > > > (https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1...) > > > > We have updated the design doc also. > > We would like to know if there are any more blockers for this CL. > > Thanks for your patience! This generally looks good to me. Unfortunately there > is still some ongoing discussion (that I only just became aware of) of the > privacy requirements for Chrome. Although it's unlikely this part of the CL will > change, I'd rather wait until those things are resolved before we land this code > if possible. We definitely won't be able to land subsequent CLs until those > things are resolved anyway. > Hi Raymes, Now that we have privacy plans for Generic Sensor in place [1,2] I guess now is a good time to revisit this CL. [1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... [2] https://docs.google.com/document/d/1XThujZ2VJm0z0Gon1zbFkYhYo6K8nMxJjxNJ3wk9K... > AFAIU owencm@ is the point of contact for status.
On 2017/07/17 15:54:08, riju_ wrote: > On 2017/04/20 03:20:52, raymes wrote: > > +owencm who is PM of this feature for Chrome > > > > On 2017/04/19 10:20:22, riju_ wrote: > > > @raymes: PR for the permission spec for this corresponding change has now > > > landed. > > > > > > (https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1...) > > > > > > We have updated the design doc also. > > > We would like to know if there are any more blockers for this CL. > > > > Thanks for your patience! This generally looks good to me. Unfortunately there > > is still some ongoing discussion (that I only just became aware of) of the > > privacy requirements for Chrome. Although it's unlikely this part of the CL > will > > change, I'd rather wait until those things are resolved before we land this > code > > if possible. We definitely won't be able to land subsequent CLs until those > > things are resolved anyway. > > > > Hi Raymes, > Now that we have privacy plans for Generic Sensor in place [1,2] I guess now is > a good time to revisit this CL. Yep thanks for your patience - I will take a look tomorrow.
mojom lgtm
lgtm, thanks! https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add ContentSetting entries for sensors later. We probably only want to add 1 ContentSettingsType and have all these map to the same.
The CQ bit was checked by rijubrata.bhaumik@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2791623004/#ps240001 (title: "Rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by rijubrata.bhaumik@intel.com
rijubrata.bhaumik@intel.com changed reviewers: + kinuko@chromium.org, tobiasjs@chromium.org
kinuko@chromium.org: Please review changes in content/public/browser tobiasjs@chromium.org: Please review changes in android_webview/browser/ https://codereview.chromium.org/2791623004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right): https://codereview.chromium.org/2791623004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/permissions/Permissions.cpp:106: IsSensorRuntimeFlagEnabled(exception_state)) On 2017/04/20 13:26:32, mlamouri (slow - plz ping) wrote: > Instead of checking for the flag N times, could you do: > ``` > if (name == "ambient-ligth-sensor" || name == "accelerometer" || name = > "gyroscope" || name == "magnetometer") { > if (!RuntimeEnabledFeatures::sensorEnabled()) { > exception_state.ThrowTypeError("GenericSensor flag is not enabled."); > return nullptr; > } > // check for all permissions > } > ``` Done.
android_webview LGTM
android_webview LGTM
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add ContentSetting entries for sensors later. On 2017/07/19 02:42:10, raymes wrote: > We probably only want to add 1 ContentSettingsType and have all these map to the > same. I'm tempted to say that even adding multiple PermissionType enum values for the different sensors adds unnecessary complexity so all we need is to support multiple string names in ParsePermission().
On 2017/07/19 21:30:27, Reilly Grant (use Gerrit) wrote: > https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... > File chrome/browser/permissions/permission_manager.cc (right): > > https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... > chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add > ContentSetting entries for sensors later. > On 2017/07/19 02:42:10, raymes wrote: > > We probably only want to add 1 ContentSettingsType and have all these map to > the > > same. > > I'm tempted to say that even adding multiple PermissionType enum values for the > different sensors adds unnecessary complexity so all we need is to support > multiple string names in ParsePermission(). That sounds like it might be a good idea for now. It would give other content embedders a bit less flexibility but it's unclear they would want that right now anyway.
The CQ bit was checked by rijubrata.bhaumik@intel.com 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.
Thanks Reilly, fixed now. https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add ContentSetting entries for sensors later. On 2017/07/19 21:30:27, Reilly Grant (use Gerrit) wrote: > On 2017/07/19 02:42:10, raymes wrote: > > We probably only want to add 1 ContentSettingsType and have all these map to > the > > same. > > I'm tempted to say that even adding multiple PermissionType enum values for the > different sensors adds unnecessary complexity so all we need is to support > multiple string names in ParsePermission(). Done.
https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:109: return CONTENT_SETTINGS_TYPE_DEFAULT; Is it valid to use this? Maybe we should combine this with the other permissions patch that is floating around and also define a ContentSettingsType for this. https://codereview.chromium.org/2791623004/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/permissions/permission.mojom (right): https://codereview.chromium.org/2791623004/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/permissions/permission.mojom:23: MAGNETOMETER, Is there any reason we need multiple enum types here either?
The CQ bit was checked by rijubrata.bhaumik@intel.com 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...
Patchset #8 (id:280001) has been deleted
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #8 (id:300001) has been deleted
Patchset #8 (id:320001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #8 (id:340001) has been deleted
The CQ bit was checked by rijubrata.bhaumik@intel.com 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...
reilly@ : Added ContentSettingType for Sensors in this patch now. Please take a look. https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:109: return CONTENT_SETTINGS_TYPE_DEFAULT; On 2017/07/20 13:56:01, Reilly Grant (use Gerrit) wrote: > Is it valid to use this? Maybe we should combine this with the other permissions > patch that is floating around and also define a ContentSettingsType for this. Done. https://codereview.chromium.org/2791623004/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/permissions/permission.mojom (right): https://codereview.chromium.org/2791623004/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/permissions/permission.mojom:23: MAGNETOMETER, On 2017/07/20 13:56:02, Reilly Grant (use Gerrit) wrote: > Is there any reason we need multiple enum types here either? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:1: nit: extra line
lgtm. We will need to add an entry in content_settings_registry.cc at some point, but we can leave it until a future CL. mlamouri may want to take a look at third_party/WebKit/Source/modules/permissions/Permissions.cpp since it has changed since the last review but it seems ok to me. https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:2: file://device/generic_sensor/OWNERS nit: please add per-file *permission_context*=set noparent per-file *permission_context*=file://chrome/browser/permissions/PERMISSIONS_OWNERS https://codereview.chromium.org/2791623004/diff/290016/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2791623004/diff/290016/components/content_set... components/content_settings/core/common/content_settings_types.h:79: // and magnetometer are all mapped to a single content_settings_type. nit: // Setting for the Generic Sensor API covering ambient-light-sensor, accelerometer, gyroscope and magnetometer. These are all mapped to a single ContentSettingsType.
Thanks raymes and reilly for the review. kinuko@ : Please have a look at a one-liner in content/public/browser/permission_type.h https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:1: On 2017/07/21 20:27:18, Reilly Grant (use Gerrit) wrote: > nit: extra line Done. https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:2: file://device/generic_sensor/OWNERS On 2017/07/24 01:54:41, raymes wrote: > nit: please add > > per-file *permission_context*=set noparent > per-file > *permission_context*=file://chrome/browser/permissions/PERMISSIONS_OWNERS Done. https://codereview.chromium.org/2791623004/diff/290016/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2791623004/diff/290016/components/content_set... components/content_settings/core/common/content_settings_types.h:79: // and magnetometer are all mapped to a single content_settings_type. On 2017/07/24 01:54:41, raymes wrote: > nit: > > // Setting for the Generic Sensor API covering ambient-light-sensor, > accelerometer, gyroscope and magnetometer. These are all mapped to a single > ContentSettingsType. Done.
lgtm
The CQ bit was checked by rijubrata.bhaumik@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, tobiasjs@chromium.org, tsepez@chromium.org, reillyg@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2791623004/#ps370001 (title: "fixing nits")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alexander.shalamov@intel.com changed reviewers: + alexander.shalamov@intel.com
https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:1: file://device/generic_sensor/OWNERS The generic_sensor component was moved to services. Should this line be updated with new component location?
https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:1: file://device/generic_sensor/OWNERS On 2017/07/24 11:30:44, shalamov wrote: > The generic_sensor component was moved to services. Should this line be updated > with new component location? Done.
lgtm
rijubrata.bhaumik@intel.com changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in chrome/browser/generic_sensor/
lgtm
https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent Why noparent?
https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent On 2017/07/25 15:21:25, jochen (gone - plz use gerrit) wrote: > Why noparent? Modifying permission_context classes has potential security implications so we're requiring a review by enamel team for modifications to those classes. This allows the subclasses to live with the feature-specific code but get the appropriate review.
On 2017/07/26 at 00:47:19, raymes wrote: > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... > File chrome/browser/generic_sensor/OWNERS (right): > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... > chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent > On 2017/07/25 15:21:25, jochen (gone - plz use gerrit) wrote: > > Why noparent? > > Modifying permission_context classes has potential security implications so we're requiring a review by enamel team for modifications to those classes. This allows the subclasses to live with the feature-specific code but get the appropriate review. eh, but it also blocks large scale refactorings :/ are you sure that the kinda of problems you'd get here are comparable to sandbox escapes (against which we try to defend with IPC reviews)?
> eh, but it also blocks large scale refactorings :/ are you sure that the kinda > of problems you'd get here are comparable to sandbox escapes (against which we > try to defend with IPC reviews)? The implications are very different to a sandbox escape and I'd prefer to avoid trying to compare potential bugs (I think you can come up with devastating bugs in any part of the code). I guess the main concern is that the right owners for the changes to these files aren't the owners of the directories where they live. Unless we set noparent then there's no reliable way to direct people to the right owners as the first point of contact. What is the process for reviewing large-scale refactorings these days? TBR should still work if it's appropriate? If they tend to get reviewed by ENG_REVIEW_OWNERS we could add file://ENG_REVIEW_OWNERS to PERMISSIONS_OWNERS? Alternatively we could move all of the subclasses into a single directory, but that doesn't seem ideal. Thanks!
The CQ bit was checked by rijubrata.bhaumik@intel.com 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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jochen@ : Friendly ping. https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent On 2017/07/26 00:47:18, raymes wrote: > On 2017/07/25 15:21:25, jochen (gone - plz use gerrit) wrote: > > Why noparent? > > Modifying permission_context classes has potential security implications so > we're requiring a review by enamel team for modifications to those classes. This > allows the subclasses to live with the feature-specific code but get the > appropriate review. Reference: https://codereview.chromium.org/2969393002
On 2017/08/01 at 12:09:33, rijubrata.bhaumik wrote: > jochen@ : Friendly ping. > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... > File chrome/browser/generic_sensor/OWNERS (right): > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... > chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent > On 2017/07/26 00:47:18, raymes wrote: > > On 2017/07/25 15:21:25, jochen (gone - plz use gerrit) wrote: > > > Why noparent? > > > > Modifying permission_context classes has potential security implications so > > we're requiring a review by enamel team for modifications to those classes. This > > allows the subclasses to live with the feature-specific code but get the > > appropriate review. > > Reference: > https://codereview.chromium.org/2969393002 let me sanity check this, I'll update the CL later
On 2017/08/01 at 12:09:33, rijubrata.bhaumik wrote: > jochen@ : Friendly ping. > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... > File chrome/browser/generic_sensor/OWNERS (right): > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic... > chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent > On 2017/07/26 00:47:18, raymes wrote: > > On 2017/07/25 15:21:25, jochen (gone - plz use gerrit) wrote: > > > Why noparent? > > > > Modifying permission_context classes has potential security implications so > > we're requiring a review by enamel team for modifications to those classes. This > > allows the subclasses to live with the feature-specific code but get the > > appropriate review. > > Reference: > https://codereview.chromium.org/2969393002 let me sanity check this, I'll update the CL later
jochen@chromium.org changed reviewers: + brettw@chromium.org
ok, so the CL except for the no-parent stuff LGTM I'm ok with landing this as is, however, please either get rid of the no-parent rules in a follow-up CL (e.g. I'd assume that using a watchlist entry + asking the owners of all the directories to ensure to loop you in for permission context changes would be an option), or alternatively, seek approval from eng-review I'll update the OWNERS documentation accordingly
On 2017/08/02 07:16:30, jochen (gone - plz use gerrit) wrote: > ok, so the CL except for the no-parent stuff LGTM > > I'm ok with landing this as is, however, please either get rid of the no-parent > rules in a follow-up CL (e.g. I'd assume that using a watchlist entry + asking > the owners of all the directories to ensure to loop you in for permission > context changes would be an option), or alternatively, seek approval from > eng-review > > I'll update the OWNERS documentation accordingly Thanks jochen! I will land this now. I think raymes being the author of (https://codereview.chromium.org/2969393002) and also part of PERMISSIONS_OWNERS will like to chime in the no-parent rules situation, as it is common for all the OWNERS file under chrome/browser/*
The CQ bit was checked by rijubrata.bhaumik@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, mlamouri@chromium.org, kinuko@chromium.org, raymes@chromium.org, tobiasjs@chromium.org, tsepez@chromium.org, alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/2791623004/#ps410001 (title: "Rebase + add blank line in ONWERS file.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'll follow up on the per-file OWNERS with eng review. On Wed, 2 Aug 2017 at 17:43 commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > CQ is trying da patch. > > Follow status at: > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > https://codereview.chromium.org/2791623004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I'll follow up on the per-file OWNERS with eng review. On Wed, 2 Aug 2017 at 17:43 commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > CQ is trying da patch. > > Follow status at: > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > https://codereview.chromium.org/2791623004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is committing da patch. Bot data: {"patchset_id": 410001, "attempt_start_ts": 1501659806799000, "parent_rev": "1216ceaf97d8da80704312e8463d8b420c4f9ea9", "commit_rev": "7b072ad1ed828f70c9255c9c863542f72d8863b9"}
Message was sent while issue was closed.
Description was changed from ========== [sensors][permission] Add new permission types in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 ========== to ========== [sensors][permission] Add new permission types in permission module. This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here https://github.com/w3c/sensors/issues/22 This CL adds permission name for the different sensors in the permission module. Spec discussion : https://github.com/w3c/permissions/pull/142 BUG=606766 Review-Url: https://codereview.chromium.org/2791623004 Cr-Commit-Position: refs/heads/master@{#491303} Committed: https://chromium.googlesource.com/chromium/src/+/7b072ad1ed828f70c9255c9c8635... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:410001) as https://chromium.googlesource.com/chromium/src/+/7b072ad1ed828f70c9255c9c8635... |