Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(276)

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

Created:
3 years, 8 months ago by riju_
Modified:
3 years, 4 months 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. ...
3 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months ago (2017-04-04 10:52:14 UTC) #16
mlamouri (slow - plz ping)
Should LayoutTests be added?
3 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months ago (2017-04-05 18:03:59 UTC) #27
riju_
tsepez@ : for permission.mojom file.
3 years, 8 months ago (2017-04-05 18:06:53 UTC) #29
Tom Sepez
lgtm
3 years, 8 months ago (2017-04-05 18:22:38 UTC) #30
riju_
@raymes: Can you take a look please.
3 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 months 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 years, 8 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 years, 8 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 years, 8 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 years, 8 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 years, 8 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 ...
3 years, 7 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months ago (2017-07-18 04:13:51 UTC) #92
Tom Sepez
mojom lgtm
3 years, 5 months 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 years, 5 months 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 years, 5 months 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 years, 5 months 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 years, 5 months ago (2017-07-19 06:56:46 UTC) #102
Tobias Sargeant
android_webview LGTM
3 years, 5 months ago (2017-07-19 10:01:38 UTC) #103
Tobias Sargeant
android_webview LGTM
3 years, 5 months 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 years, 5 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): > ...
3 years, 5 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 ...
3 years, 5 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 ...
3 years, 5 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 ...
3 years, 5 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
3 years, 5 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 ...
3 years, 5 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 ...
3 years, 5 months ago (2017-07-24 07:19:34 UTC) #132
kinuko
lgtm
3 years, 5 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
3 years, 5 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)
3 years, 5 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 ...
3 years, 5 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 ...
3 years, 5 months ago (2017-07-24 11:36:08 UTC) #141
shalamov
lgtm
3 years, 5 months ago (2017-07-24 11:36:42 UTC) #142
riju_
jochen@chromium.org: Please review changes in chrome/browser/generic_sensor/
3 years, 5 months ago (2017-07-24 11:50:18 UTC) #144
Mikhail
lgtm
3 years, 5 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?
3 years, 5 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 - ...
3 years, 5 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 years, 4 months ago (2017-07-27 20:48:13 UTC) #148
raymes
> eh, but it also blocks large scale refactorings :/ are you sure that the ...
3 years, 4 months 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 years, 4 months 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 years, 4 months 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 years, 4 months 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 years, 4 months 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 years, 4 months 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 years, 4 months 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 years, 4 months 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 years, 4 months ago (2017-08-02 07:45:53 UTC) #164
commit-bot: I haz the power
3 years, 4 months 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 408576698