Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 3 weeks ago by riju_
Modified:
1 day, 6 hours 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, shalamov, Reilly Grant (use Gerrit)
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: 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: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -2 lines) Patch
M android_webview/browser/aw_permission_manager.cc View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/generic_sensor/OWNERS View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 1 comment Download
A chrome/browser/generic_sensor/sensor_permission_context.h View 1 2 3 4 5 6 7 1 chunk +28 lines, -0 lines 0 comments Download
A chrome/browser/generic_sensor/sensor_permission_context.cc View 1 2 3 4 5 6 7 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/permissions/permission_manager.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/permissions/permission_util.cc View 1 2 3 4 5 6 7 4 chunks +7 lines, -0 lines 0 comments Download
M components/content_settings/core/common/content_settings.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M components/content_settings/core/common/content_settings_types.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/permissions/permission_service_impl.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M content/public/browser/permission_type.h View 1 2 3 4 5 6 1 chunk +1 line, -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 5 6 7 2 chunks +11 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/permissions/permission.mojom View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 130 (89 generated)
riju_
Mounir : For some context, this is the WIP CL (https://codereview.chromium.org/2458453002/) for some overall picture. ...
3 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks ago (2017-04-04 10:52:14 UTC) #16
mlamouri (slow - plz ping)
Should LayoutTests be added?
3 months, 2 weeks 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 months, 2 weeks ago (2017-04-04 15:12:02 UTC) #20
mlamouri (slow - plz ping)
sgtm but isn't this exposing the feature to the web?
3 months, 2 weeks 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 months, 2 weeks ago (2017-04-05 13:33:01 UTC) #24
mlamouri (slow - plz ping)
On 2017/04/05 at 13:33:01, rijubrata.bhaumik wrote: > On 2017/04/05 13:17:22, mlamouri wrote: > > sgtm ...
3 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks ago (2017-04-05 18:03:59 UTC) #27
riju_
tsepez@ : for permission.mojom file.
3 months, 2 weeks ago (2017-04-05 18:06:53 UTC) #29
Tom Sepez
lgtm
3 months, 2 weeks ago (2017-04-05 18:22:38 UTC) #30
riju_
@raymes: Can you take a look please.
3 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks ago (2017-04-07 07:37:30 UTC) #33
mlamouri (slow - plz ping)
On 2017/04/07 at 07:37:30, alexander.shalamov wrote: > @mlamouri > > Do you have any ideas ...
3 months, 2 weeks 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 ...
3 months, 1 week 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 ...
3 months 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: > ...
3 months 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 ...
3 months 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 ...
3 months ago (2017-04-20 03:53:10 UTC) #70
mlamouri (slow - plz ping)
lgtm assuming raymes@ is happy with it https://codereview.chromium.org/2791623004/diff/160001/third_party/WebKit/Source/modules/permissions/Permissions.cpp File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right): https://codereview.chromium.org/2791623004/diff/160001/third_party/WebKit/Source/modules/permissions/Permissions.cpp#newcode106 third_party/WebKit/Source/modules/permissions/Permissions.cpp:106: IsSensorRuntimeFlagEnabled(exception_state)) Instead ...
3 months ago (2017-04-20 13:26:32 UTC) #71
riju_
@raymes, @owen: After getting to know the concerns from Privacy team, regarding low and high ...
2 months, 2 weeks ago (2017-05-03 13:43:22 UTC) #76
riju_
On 2017/04/20 03:20:52, raymes wrote: > +owencm who is PM of this feature for Chrome ...
5 days, 10 hours ago (2017-07-17 15:54:08 UTC) #91
raymes
On 2017/07/17 15:54:08, riju_ wrote: > On 2017/04/20 03:20:52, raymes wrote: > > +owencm who ...
4 days, 22 hours ago (2017-07-18 04:13:51 UTC) #92
Tom Sepez
mojom lgtm
4 days, 4 hours ago (2017-07-18 22:21:48 UTC) #93
raymes
lgtm, thanks! https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc#newcode107 chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add ContentSetting entries for sensors ...
3 days, 23 hours ago (2017-07-19 02:42:10 UTC) #94
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2791623004/240001
3 days, 20 hours ago (2017-07-19 06:37:02 UTC) #97
commit-bot: I haz the power
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_presubmit/builds/493491)
3 days, 19 hours ago (2017-07-19 06:46:24 UTC) #99
riju_
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/Source/modules/permissions/Permissions.cpp File third_party/WebKit/Source/modules/permissions/Permissions.cpp ...
3 days, 19 hours ago (2017-07-19 06:56:46 UTC) #102
Tobias Sargeant
android_webview LGTM
3 days, 16 hours ago (2017-07-19 10:01:38 UTC) #103
Tobias Sargeant
android_webview LGTM
3 days, 16 hours ago (2017-07-19 10:01:41 UTC) #104
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc#newcode107 chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add ContentSetting entries for sensors later. On ...
3 days, 5 hours ago (2017-07-19 21:30:27 UTC) #106
raymes
On 2017/07/19 21:30:27, Reilly Grant (use Gerrit) wrote: > https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc > File chrome/browser/permissions/permission_manager.cc (right): > ...
3 days, 2 hours ago (2017-07-19 23:45:00 UTC) #107
riju_
Thanks Reilly, fixed now. https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/240001/chrome/browser/permissions/permission_manager.cc#newcode107 chrome/browser/permissions/permission_manager.cc:107: // TODO(riju): Add ContentSetting entries ...
2 days, 15 hours ago (2017-07-20 11:07:41 UTC) #112
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permissions/permission_manager.cc File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permissions/permission_manager.cc#newcode109 chrome/browser/permissions/permission_manager.cc:109: return CONTENT_SETTINGS_TYPE_DEFAULT; Is it valid to use this? Maybe ...
2 days, 12 hours ago (2017-07-20 13:56:02 UTC) #113
riju_
reilly@ : Added ContentSettingType for Sensors in this patch now. Please take a look. https://codereview.chromium.org/2791623004/diff/260001/chrome/browser/permissions/permission_manager.cc ...
1 day, 13 hours ago (2017-07-21 13:03:38 UTC) #127
Reilly Grant (use Gerrit)
1 day, 6 hours ago (2017-07-21 20:27:19 UTC) #130
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
Sign in to reply to this message.

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