Add API_OWNERS to RuntimeEnabledFeatures OWNERS

Add API_OWNERS to RuntimeEnabledFeatures OWNERS

Committed: https://crrev.com/e0fa28b7e05e2a8cdc35eca7a916d39a4459ed5c
Cr-Commit-Position: refs/heads/master@{#434533}

[Rick Byers, 2016-11-23 18:28:58]

rbyers@chromium.org changed reviewers:
+ haraken@chromium.org

[Rick Byers, 2016-11-23 18:28:59]

Kentarosan, PTAL

This is technically a no-op due to the API_OWNERS rule in WebKit/OWNERS.  But
I've seen a number of CLs where a platform OWNER is added just to review a
RuntimeEnabledFeatures.in change when there's already an API_OWNER reviewing the
CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully this will
help reduce that confusion.

[Rick Byers, 2016-11-23 18:33:02]

On 2016/11/23 18:28:59, Rick Byers wrote:
> Kentarosan, PTAL
> 
> This is technically a no-op due to the API_OWNERS rule in WebKit/OWNERS.  But
> I've seen a number of CLs where a platform OWNER is added just to review a
> RuntimeEnabledFeatures.in change when there's already an API_OWNER reviewing
the
> CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully this
will
> help reduce that confusion.

BTW, I was tempted to suggest that maybe this file should have '*' ownership
(changes covered by whoever is reviewing the usage, eg. from
Source/core/OWNERS).  But perhaps it's best continue having extra scrutiny
specifically on the introduction/change of a RuntimeEnabledFeature?

[haraken, 2016-11-24 01:56:02]

On 2016/11/23 18:33:02, Rick Byers wrote:
> On 2016/11/23 18:28:59, Rick Byers wrote:
> > Kentarosan, PTAL
> > 
> > This is technically a no-op due to the API_OWNERS rule in WebKit/OWNERS. 
But
> > I've seen a number of CLs where a platform OWNER is added just to review a
> > RuntimeEnabledFeatures.in change when there's already an API_OWNER reviewing
> the
> > CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully this
> will
> > help reduce that confusion.
> 
> BTW, I was tempted to suggest that maybe this file should have '*' ownership
> (changes covered by whoever is reviewing the usage, eg. from
> Source/core/OWNERS).  But perhaps it's best continue having extra scrutiny
> specifically on the introduction/change of a RuntimeEnabledFeature?

Would it make more sense to move RuntimeEnabledFeatures.in to Source/?

[Rick Byers, 2016-11-25 15:14:29]

On 2016/11/24 at 01:56:02, haraken wrote:
> On 2016/11/23 18:33:02, Rick Byers wrote:
> > On 2016/11/23 18:28:59, Rick Byers wrote:
> > > Kentarosan, PTAL
> > > 
> > > This is technically a no-op due to the API_OWNERS rule in WebKit/OWNERS. 
But
> > > I've seen a number of CLs where a platform OWNER is added just to review a
> > > RuntimeEnabledFeatures.in change when there's already an API_OWNER
reviewing
> > the
> > > CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully
this
> > will
> > > help reduce that confusion.
> > 
> > BTW, I was tempted to suggest that maybe this file should have '*' ownership
> > (changes covered by whoever is reviewing the usage, eg. from
> > Source/core/OWNERS).  But perhaps it's best continue having extra scrutiny
> > specifically on the introduction/change of a RuntimeEnabledFeature?
> 
> Would it make more sense to move RuntimeEnabledFeatures.in to Source/?

You mean move it to Source/core?  We can't do that because there are things
below core/ that need to use the generated RuntimeEnabledFeatures.h (like stuff
in platform/).

Or did you really mean to make a new component in Source/ itself that platform,
core, etc. can depend on?  We'd still have the problem of defining an
appropriate OWNERS file etc.  So I don't think that would buy us anything.

[haraken, 2016-11-25 15:24:37]

On 2016/11/25 15:14:29, Rick Byers wrote:
> On 2016/11/24 at 01:56:02, haraken wrote:
> > On 2016/11/23 18:33:02, Rick Byers wrote:
> > > On 2016/11/23 18:28:59, Rick Byers wrote:
> > > > Kentarosan, PTAL
> > > > 
> > > > This is technically a no-op due to the API_OWNERS rule in WebKit/OWNERS.

> But
> > > > I've seen a number of CLs where a platform OWNER is added just to review
a
> > > > RuntimeEnabledFeatures.in change when there's already an API_OWNER
> reviewing
> > > the
> > > > CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully
> this
> > > will
> > > > help reduce that confusion.
> > > 
> > > BTW, I was tempted to suggest that maybe this file should have '*'
ownership
> > > (changes covered by whoever is reviewing the usage, eg. from
> > > Source/core/OWNERS).  But perhaps it's best continue having extra scrutiny
> > > specifically on the introduction/change of a RuntimeEnabledFeature?
> > 
> > Would it make more sense to move RuntimeEnabledFeatures.in to Source/?
> 
> You mean move it to Source/core?  We can't do that because there are things
> below core/ that need to use the generated RuntimeEnabledFeatures.h (like
stuff
> in platform/).
> 
> Or did you really mean to make a new component in Source/ itself that
platform,
> core, etc. can depend on?  We'd still have the problem of defining an
> appropriate OWNERS file etc.  So I don't think that would buy us anything.

I was thinking about putting RuntimeEnabledFeatures.in in Source/ but generate
RuntimeEnabledFeatures.h/cpp in the platform/. Is it too crazy?

If it's crazy, this CL LGTM.

[Rick Byers, 2016-11-25 16:36:11]

On 2016/11/25 at 15:24:37, haraken wrote:
> On 2016/11/25 15:14:29, Rick Byers wrote:
> > On 2016/11/24 at 01:56:02, haraken wrote:
> > > On 2016/11/23 18:33:02, Rick Byers wrote:
> > > > On 2016/11/23 18:28:59, Rick Byers wrote:
> > > > > Kentarosan, PTAL
> > > > > 
> > > > > This is technically a no-op due to the API_OWNERS rule in
WebKit/OWNERS. 
> > But
> > > > > I've seen a number of CLs where a platform OWNER is added just to
review a
> > > > > RuntimeEnabledFeatures.in change when there's already an API_OWNER
> > reviewing
> > > > the
> > > > > CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully
> > this
> > > > will
> > > > > help reduce that confusion.
> > > > 
> > > > BTW, I was tempted to suggest that maybe this file should have '*'
ownership
> > > > (changes covered by whoever is reviewing the usage, eg. from
> > > > Source/core/OWNERS).  But perhaps it's best continue having extra
scrutiny
> > > > specifically on the introduction/change of a RuntimeEnabledFeature?
> > > 
> > > Would it make more sense to move RuntimeEnabledFeatures.in to Source/?
> > 
> > You mean move it to Source/core?  We can't do that because there are things
> > below core/ that need to use the generated RuntimeEnabledFeatures.h (like
stuff
> > in platform/).
> > 
> > Or did you really mean to make a new component in Source/ itself that
platform,
> > core, etc. can depend on?  We'd still have the problem of defining an
> > appropriate OWNERS file etc.  So I don't think that would buy us anything.
> 
> I was thinking about putting RuntimeEnabledFeatures.in in Source/ but generate
RuntimeEnabledFeatures.h/cpp in the platform/. Is it too crazy?

I guess we could do that, but it would be pretty weird - right? All those
#include "platform/RuntimeEnabledFeatures.h" would be harder for devs to reason
about ("where did this come from?").  I also don't think we have a need to lock
down the ownership of RuntimeEnabledFeatures.in changes to just API_OWNERS -
platform/OWNERS seems to be great other than the confusion here.
 
> If it's crazy, this CL LGTM.

Ok let's do this for now, but happy to keep discussing additional ideas for
making this better...

[Rick Byers, 2016-11-25 16:36:15]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2016-11-25 16:36:27]

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, 2016-11-25 17:28:28]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480091775075410, "parent_rev":
"fa329342d55504e7fecded7ad071907319321b82", "commit_rev":
"2aba817318a9100d541a636d14b04c1027e6bcc9"}

[commit-bot: I haz the power, 2016-11-25 17:28:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-25 17:31:25]

Description was changed from

==========
Add API_OWNERS to RuntimeEnabledFeatures OWNERS
==========

to

==========
Add API_OWNERS to RuntimeEnabledFeatures OWNERS

Committed: https://crrev.com/e0fa28b7e05e2a8cdc35eca7a916d39a4459ed5c
Cr-Commit-Position: refs/heads/master@{#434533}
==========

[commit-bot: I haz the power, 2016-11-25 17:31:26]

Patchset 1 (id:??) landed as
https://crrev.com/e0fa28b7e05e2a8cdc35eca7a916d39a4459ed5c
Cr-Commit-Position: refs/heads/master@{#434533}

[jbroman, 2016-11-25 17:41:34]

On 2016/11/23 at 18:28:59, rbyers wrote:
> This is technically a no-op due to the API_OWNERS rule in WebKit/OWNERS.  But
I've seen a number of CLs where a platform OWNER is added just to review a
RuntimeEnabledFeatures.in change when there's already an API_OWNER reviewing the
CL (eg. https://codereview.chromium.org/2525813003/#msg12).  Hopefully this will
help reduce that confusion.

FYI, I've seen a number of similar cases arising from the use of extensions
(like Chromite Butler) that implement an incomplete subset of the OWNERS file
logic (in particular, not expanding file: references). I'm not sure what to do
about that, aside from possibly asking the PolyGerrit folks to support such a
feature so that we don't have that kind of issue in the future (crbug.com/604736
seems to cover that).