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

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

Created:
7 months, 3 weeks ago by riju_
Modified:
3 months, 3 weeks ago
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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+143 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 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/generic_sensor/OWNERS View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -0 lines 0 comments 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 8 9 10 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 8 9 10 1 chunk +7 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

Messages

Total messages: 167 (103 generated)
riju_
Mounir : For some context, this is the WIP CL (https://codereview.chromium.org/2458453002/) for some overall picture. ...
7 months, 3 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 ...
7 months, 3 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 ...
7 months, 3 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 ...
7 months, 3 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 ...
7 months, 3 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: ...
7 months, 3 weeks ago (2017-04-04 10:52:14 UTC) #16
mlamouri (slow - plz ping)
Should LayoutTests be added?
7 months, 3 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 ...
7 months, 3 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?
7 months, 3 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 ...
7 months, 3 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 ...
7 months, 3 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 ...
7 months, 3 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 ...
7 months, 3 weeks ago (2017-04-05 18:03:59 UTC) #27
riju_
tsepez@ : for permission.mojom file.
7 months, 3 weeks ago (2017-04-05 18:06:53 UTC) #29
Tom Sepez
lgtm
7 months, 3 weeks ago (2017-04-05 18:22:38 UTC) #30
riju_
@raymes: Can you take a look please.
7 months, 3 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 ...
7 months, 3 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 ...
7 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 ...
7 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 ...
7 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 ...
7 months, 1 week 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: > ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 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 ...
6 months, 3 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 ...
4 months, 1 week 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 months, 1 week ago (2017-07-18 04:13:51 UTC) #92
Tom Sepez
mojom lgtm
4 months, 1 week 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 ...
4 months, 1 week 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
4 months, 1 week 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)
4 months, 1 week 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 ...
4 months, 1 week ago (2017-07-19 06:56:46 UTC) #102
Tobias Sargeant
android_webview LGTM
4 months, 1 week ago (2017-07-19 10:01:38 UTC) #103
Tobias Sargeant
android_webview LGTM
4 months, 1 week 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 ...
4 months 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): > ...
4 months 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 ...
4 months 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 ...
4 months 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 ...
4 months ago (2017-07-21 13:03:38 UTC) #127
Reilly Grant (use Gerrit)
lgtm https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic_sensor/OWNERS File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/290016/chrome/browser/generic_sensor/OWNERS#newcode1 chrome/browser/generic_sensor/OWNERS:1: nit: extra line
4 months ago (2017-07-21 20:27:19 UTC) #130
raymes
lgtm. We will need to add an entry in content_settings_registry.cc at some point, but we ...
4 months ago (2017-07-24 01:54:41 UTC) #131
riju_
Thanks raymes and reilly for the review. kinuko@ : Please have a look at a ...
4 months ago (2017-07-24 07:19:34 UTC) #132
kinuko
lgtm
4 months ago (2017-07-24 10:34:43 UTC) #133
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/370001
4 months ago (2017-07-24 10:49:21 UTC) #136
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/497498)
4 months ago (2017-07-24 10:59:32 UTC) #138
shalamov
https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic_sensor/OWNERS File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic_sensor/OWNERS#newcode1 chrome/browser/generic_sensor/OWNERS:1: file://device/generic_sensor/OWNERS The generic_sensor component was moved to services. Should ...
4 months ago (2017-07-24 11:30:45 UTC) #140
riju_
https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic_sensor/OWNERS File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/370001/chrome/browser/generic_sensor/OWNERS#newcode1 chrome/browser/generic_sensor/OWNERS:1: file://device/generic_sensor/OWNERS On 2017/07/24 11:30:44, shalamov wrote: > The generic_sensor ...
4 months ago (2017-07-24 11:36:08 UTC) #141
shalamov
lgtm
4 months ago (2017-07-24 11:36:42 UTC) #142
riju_
jochen@chromium.org: Please review changes in chrome/browser/generic_sensor/
4 months ago (2017-07-24 11:50:18 UTC) #144
Mikhail
lgtm
4 months ago (2017-07-24 12:17:50 UTC) #145
jochen (gone - plz use gerrit)
https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS#newcode2 chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent Why noparent?
4 months ago (2017-07-25 15:21:26 UTC) #146
raymes
https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS#newcode2 chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent On 2017/07/25 15:21:25, jochen (gone - ...
4 months ago (2017-07-26 00:47:19 UTC) #147
jochen (gone - plz use gerrit)
On 2017/07/26 at 00:47:19, raymes wrote: > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS > File chrome/browser/generic_sensor/OWNERS (right): > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS#newcode2 ...
3 months, 4 weeks ago (2017-07-27 20:48:13 UTC) #148
raymes
> eh, but it also blocks large scale refactorings :/ are you sure that the ...
3 months, 3 weeks ago (2017-07-31 06:57:00 UTC) #149
riju_
jochen@ : Friendly ping. https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS File chrome/browser/generic_sensor/OWNERS (right): https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS#newcode2 chrome/browser/generic_sensor/OWNERS:2: per-file *permission_context*=set noparent On 2017/07/26 ...
3 months, 3 weeks ago (2017-08-01 12:09:33 UTC) #154
jochen (gone - plz use gerrit)
On 2017/08/01 at 12:09:33, rijubrata.bhaumik wrote: > jochen@ : Friendly ping. > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS > ...
3 months, 3 weeks ago (2017-08-01 19:56:29 UTC) #155
jochen (gone - plz use gerrit)
On 2017/08/01 at 12:09:33, rijubrata.bhaumik wrote: > jochen@ : Friendly ping. > > https://codereview.chromium.org/2791623004/diff/390001/chrome/browser/generic_sensor/OWNERS > ...
3 months, 3 weeks ago (2017-08-01 19:56:35 UTC) #156
jochen (gone - plz use gerrit)
ok, so the CL except for the no-parent stuff LGTM I'm ok with landing this ...
3 months, 3 weeks ago (2017-08-02 07:16:30 UTC) #158
riju_
On 2017/08/02 07:16:30, jochen (gone - plz use gerrit) wrote: > ok, so the CL ...
3 months, 3 weeks ago (2017-08-02 07:42:34 UTC) #159
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/410001
3 months, 3 weeks ago (2017-08-02 07:43:33 UTC) #162
raymes
I'll follow up on the per-file OWNERS with eng review. On Wed, 2 Aug 2017 ...
3 months, 3 weeks ago (2017-08-02 07:45:52 UTC) #163
raymes
I'll follow up on the per-file OWNERS with eng review. On Wed, 2 Aug 2017 ...
3 months, 3 weeks ago (2017-08-02 07:45:53 UTC) #164
commit-bot: I haz the power
3 months, 3 weeks ago (2017-08-02 07:49:55 UTC) #167
Message was sent while issue was closed.
Committed patchset #11 (id:410001) as
https://chromium.googlesource.com/chromium/src/+/7b072ad1ed828f70c9255c9c8635...

Powered by Google App Engine
This is Rietveld efc10ee0f