Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass changes

Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass changes

This change ensures that changes to permissions receive some sign-off
from Security UX in a similar way to IPC OWNERS.

Review-Url: https://codereview.chromium.org/2969393002
Cr-Commit-Position: refs/heads/master@{#485770}
Committed: https://chromium.googlesource.com/chromium/src/+/3136d7adfced4dd58113059f568e7112c5ca3ab7

[raymes, 2017-07-06 06:07:13]

raymes@chromium.org changed reviewers:
+ benwells@chromium.org

[raymes, 2017-07-06 06:07:14]

We should also add a PRESUBMIT check to ensure that new subclasses that are
added also set OWNERS appropriately, but this may be a good first step. wdyt?

[benwells, 2017-07-06 07:21:00]

lgtm

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/PERMISSIONS_OWNERS (right):

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/PERMISSIONS_OWNERS:1: # permission_context subclasses
require a Security UX review by one of the
Nit: PermissionContext subclasses...

[Peter Beverloo, 2017-07-06 11:29:33]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2017-07-06 11:29:34]

notifications lgtm

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/PERMISSIONS_OWNERS (right):

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/PERMISSIONS_OWNERS:8: timloh@chromium.org
If you're going down the route of `set noparent` (which we have a general
dislike for, but you know that), it's good practice to try to grow a more
geographically distributed set of OWNERS.

[mlamouri (slow - plz ping), 2017-07-07 09:40:45]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-07-07 09:40:46]

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/PERMISSIONS_OWNERS (right):

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/PERMISSIONS_OWNERS:8: timloh@chromium.org
On 2017/07/06 at 11:29:34, Peter Beverloo wrote:
> If you're going down the route of `set noparent` (which we have a general
dislike for, but you know that), it's good practice to try to grow a more
geographically distributed set of OWNERS.

If that's an issue, I'm pretty sure I can review the PermissionContext
subclasses.

[raymes, 2017-07-11 01:57:34]

raymes@chromium.org changed reviewers:
+ bauerb@chromium.org, iclelland@chromium.org, jsbell@chromium.org,
sergeyu@chromium.org

[raymes, 2017-07-11 01:57:35]

+OWNERS

sergeyu:
  chrome/browser/media/OWNERS
  chrome/browser/media/webrtc/OWNERS
bauerb:
  chrome/browser/plugins/OWNERS
jsbell:
  chrome/browser/storage/OWNERS
iclelland:
  chrome/browser/background_sync/OWNERS

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/PERMISSIONS_OWNERS (right):

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/PERMISSIONS_OWNERS:1: # permission_context subclasses
require a Security UX review by one of the
On 2017/07/06 07:20:59, benwells wrote:
> Nit: PermissionContext subclasses...

Done.

https://codereview.chromium.org/2969393002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/PERMISSIONS_OWNERS:8: timloh@chromium.org
Good point - I didn't think about that. I think changes to these files tend to
be quite rare by non-permissions folks right now so having a geographical
distribution might not be too important yet. A quick look shows only about 5-6
changes in the last 6 months by non-permissions OWNERS. Given some of the
nuances in the code it might be good to have a smaller set of owners for the
time being but we can expand that if needed :)

[Do not use (sergeyu), 2017-07-11 02:55:39]

sergeyu@google.com changed reviewers:
+ sergeyu@google.com

[Do not use (sergeyu), 2017-07-11 02:55:40]

chrome/browser/media LGTM

[Do not use (sergeyu), 2017-07-11 02:55:53]

Description was changed from

==========
Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass changes

This change ensures that changes to permissions receive some sign-off
from Security UX in a similar way to IPC OWNERS.
==========

to

==========
Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass changes

This change ensures that changes to permissions receive some sign-off
from Security UX in a similar way to IPC OWNERS.
==========

[Do not use (sergeyu), 2017-07-11 02:55:53]

sergeyu@google.com changed reviewers:
- bauerb@chromium.org, iclelland@chromium.org, jsbell@chromium.org,
sergeyu@chromium.org, sergeyu@google.com

[Sergey Ulanov, 2017-07-11 02:56:19]

LGTM from the right account

[iclelland, 2017-07-11 03:22:48]

iclelland@chromium.org changed reviewers:
+ iclelland@chromium.org

[iclelland, 2017-07-11 03:22:48]

background_sync LGTM, thanks.

(Did you mean to add jsbell and bauerb to the reviewers list as well?)

[raymes, 2017-07-11 03:24:39]

raymes@chromium.org changed reviewers:
+ bauerb@chromium.org, jsbell@chromium.org

[raymes, 2017-07-11 03:24:40]

Oops, yes. +OWNERS

bauerb:
  chrome/browser/plugins/OWNERS
jsbell:
  chrome/browser/storage/OWNERS

[Bernhard Bauer, 2017-07-11 09:47:08]

plugins LGTM

[jsbell, 2017-07-11 17:39:49]

chrome/browser/storage lgtm

[raymes, 2017-07-11 23:34:55]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-07-11 23:34:56]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2969393002/#ps40001
(title: "Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass
changes")

[commit-bot: I haz the power, 2017-07-11 23:35:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 23:40:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 23:40:36]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[raymes, 2017-07-11 23:52:06]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-07-11 23:52:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sergeyu@google.com, benwells@chromium.org, jsbell@chromium.org,
peter@chromium.org, sergeyu@chromium.org, iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2969393002/#ps60001
(title: "Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass
changes")

[commit-bot: I haz the power, 2017-07-11 23:52:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 01:29:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499817126459900,
"parent_rev": "a1b45802a88101e23d1c0e485165255807702ca7", "commit_rev":
"3136d7adfced4dd58113059f568e7112c5ca3ab7"}

[commit-bot: I haz the power, 2017-07-12 01:29:29]

Description was changed from

==========
Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass changes

This change ensures that changes to permissions receive some sign-off
from Security UX in a similar way to IPC OWNERS.
==========

to

==========
Add PERMISSIONS_OWNERS for reviewing PermissionContext subclass changes

This change ensures that changes to permissions receive some sign-off
from Security UX in a similar way to IPC OWNERS.

Review-Url: https://codereview.chromium.org/2969393002
Cr-Commit-Position: refs/heads/master@{#485770}
Committed:
https://chromium.googlesource.com/chromium/src/+/3136d7adfced4dd58113059f568e...
==========

[commit-bot: I haz the power, 2017-07-12 01:29:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3136d7adfced4dd58113059f568e...