Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2791623004: [sensors][permission] Add new permission types in permission module.

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 weeks ago by riju_
Modified:
1 week, 1 day ago
CC:
Aaron Boodman, abarth-chromium, 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, 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

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: 1

Patch Set 5 : Fix mounir's comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -1 line) Patch
M android_webview/browser/aw_permission_manager.cc View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/permissions/permission_manager.cc View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M content/browser/permissions/permission_service_impl.cc View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M content/public/browser/permission_type.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/permissions/resources/test-query.js View 1 2 3 1 chunk +36 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/permissions/PermissionDescriptor.idl View 1 2 3 1 chunk +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/permissions/Permissions.cpp View 1 2 3 4 2 chunks +18 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/permissions/permission.mojom View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 75 (50 generated)
riju_
Mounir : For some context, this is the WIP CL (https://codereview.chromium.org/2458453002/) for some overall picture. ...
3 weeks, 5 days ago (2017-04-03 06:37:38 UTC) #9
timvolodine
A comment/question below. Also, do we need all the permission types? I understood from my ...
3 weeks, 4 days ago (2017-04-03 16:57:17 UTC) #10
riju_
On 2017/04/03 16:57:17, timvolodine wrote: > A comment/question below. Also, do we need all the ...
3 weeks, 4 days ago (2017-04-03 18:01:59 UTC) #11
riju_
Adding @raymes, @benwells from the chrome privacy team, to make sure we are all in ...
3 weeks, 4 days ago (2017-04-03 18:04:32 UTC) #13
benwells
A couple of comments: - I think the description of this CL is misleading. Can ...
3 weeks, 4 days ago (2017-04-04 00:49:37 UTC) #14
riju_
Thanks for the comments ! On 2017/04/04 00:49:37, benwells wrote: > A couple of comments: ...
3 weeks, 4 days ago (2017-04-04 10:52:14 UTC) #16
mlamouri - OOO until May 8th
Should LayoutTests be added?
3 weeks, 4 days ago (2017-04-04 12:54:26 UTC) #17
riju_
On 2017/04/04 12:54:26, mlamouri wrote: > Should LayoutTests be added? I added some basic tests ...
3 weeks, 4 days ago (2017-04-04 15:12:02 UTC) #20
mlamouri - OOO until May 8th
sgtm but isn't this exposing the feature to the web?
3 weeks, 3 days ago (2017-04-05 13:17:22 UTC) #23
riju_
On 2017/04/05 13:17:22, mlamouri wrote: > sgtm but isn't this exposing the feature to the ...
3 weeks, 3 days ago (2017-04-05 13:33:01 UTC) #24
mlamouri - OOO until May 8th
On 2017/04/05 at 13:33:01, rijubrata.bhaumik wrote: > On 2017/04/05 13:17:22, mlamouri wrote: > > sgtm ...
3 weeks, 2 days ago (2017-04-05 16:11:31 UTC) #25
riju_
On 2017/04/05 16:11:31, mlamouri wrote: > On 2017/04/05 at 13:33:01, rijubrata.bhaumik wrote: > > On ...
3 weeks, 2 days ago (2017-04-05 18:01:37 UTC) #26
riju_
On 2017/04/05 18:01:37, riju_ wrote: > On 2017/04/05 16:11:31, mlamouri wrote: > > On 2017/04/05 ...
3 weeks, 2 days ago (2017-04-05 18:03:59 UTC) #27
riju_
tsepez@ : for permission.mojom file.
3 weeks, 2 days ago (2017-04-05 18:06:53 UTC) #29
Tom Sepez
lgtm
3 weeks, 2 days ago (2017-04-05 18:22:38 UTC) #30
riju_
@raymes: Can you take a look please.
3 weeks, 2 days ago (2017-04-05 21:16:04 UTC) #31
raymes
Hey riju, I'm not sure if benwells/timvolodine questions have been addressed sufficiently. Could you add ...
3 weeks, 2 days ago (2017-04-06 05:32:55 UTC) #32
shalamov
On 2017/04/06 05:32:55, raymes wrote: > Hey riju, I'm not sure if benwells/timvolodine questions have ...
3 weeks, 1 day ago (2017-04-07 07:37:30 UTC) #33
mlamouri - OOO until May 8th
On 2017/04/07 at 07:37:30, alexander.shalamov wrote: > @mlamouri > > Do you have any ideas ...
3 weeks, 1 day ago (2017-04-07 11:28:26 UTC) #34
Mikhail
raymes@, we have updated the design doc with the permissions considerations (https://docs.google.com/document/d/1Ml65ZdW5AgIsZTszk4mD_ohr40pcrdVFOIf0ZtWxDv0). After we got ...
2 weeks, 2 days ago (2017-04-13 11:59:30 UTC) #60
riju_
@raymes: PR for the permission spec for this corresponding change has now landed. (https://github.com/w3c/permissions/commit/e9623d75dad47a992597848e86607fa39c1e32d7) We ...
1 week, 3 days ago (2017-04-19 10:20:22 UTC) #66
raymes
+owencm who is PM of this feature for Chrome On 2017/04/19 10:20:22, riju_ wrote: > ...
1 week, 2 days ago (2017-04-20 03:20:52 UTC) #68
blink-reviews
Expanding a little, there's a reasonable chance Chrome may want to explicitly differentiate between a ...
1 week, 2 days ago (2017-04-20 03:53:10 UTC) #69
chromium-reviews
Expanding a little, there's a reasonable chance Chrome may want to explicitly differentiate between a ...
1 week, 2 days ago (2017-04-20 03:53:10 UTC) #70
mlamouri - OOO until May 8th
1 week, 2 days ago (2017-04-20 13:26:32 UTC) #71
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
}
```
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46