Finer-grained OWNERS

[Adam Barth]

Currently Blink has a large number of OWNERS in its top-level directory, which makes it difficult to carve off modules like devtools that should have a smaller set of OWNERS.  We could use the "no parent" directive to carve off these modules, but a number of people I've talked with who are familiar with the use of OWNERS in Chromium and internally at Google recommend trying to avoid the "no parent" directive.

Instead, I think it makes more sense to move the top-level OWNERS file to the root of the Core directory (i.e., the directory that used to be called WebCore).  Because Core is so interconnected, I think it makes sense to have common set of OWNERS for all of Core

Moving the top-level OWNERS file to Core also lets have OWNERS files for individual modules, such as IndexedDB.  Because these modules are factored out of Core, the contributors responsible for these modules can meaningfully develop them without making many changes to Core.

I've written up a CL that implements this approach.  I've tried to seed the non-Core directories with reasonable lists of OWNERS, but I'm sure I didn't get all of them exactly right.  If folks are supportive of the general approach, we can iterate in subsequent CLs:

https://codereview.chromium.org/14328010

Thanks,

Adam

[Michael Nordman]

Nice! We could probably also carve up 'core' somewhat over time...  /loader vs /rendering vs /workers type of thing.

[James Robinson]

On Wed, Apr 17, 2013 at 12:29 PM, Adam Barth <aba...@chromium.org> wrote:

I'm a big fan of this approach.  What is the process for updating the OWNERS set for a given set of directories?  Is it up to the existing OWNERS to figure this out, or should we define a set of project-wide norms for this process?

- James

https://codereview.chromium.org/14328010

Thanks,

Adam

[Emil A Eklund]

I like the division and think you've managed to strike a good balance
between separation and convenience.

Until we move unit tests out of the Tools/TestWebKitAPI it probably
makes sense to have a special OWNERS file for that directory though.

--
Emil

[David Levin]

Small nit: Is * too permissive for tools/scripts?

dave

[Jochen Eisinger]

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

best

-jochen

[James Robinson]

On Wed, Apr 17, 2013 at 12:52 PM, Jochen Eisinger <joc...@chromium.org> wrote:

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

Source/WebKit/chromium/public/OWNERS and Source/Platform/chromium/public/OWNERS, which tkent@ is in, are not changed in this patch so he's still an owner for public API.

- James

[Jochen Eisinger]

On Wed, Apr 17, 2013 at 9:54 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 12:52 PM, Jochen Eisinger <joc...@chromium.org> wrote:

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

Source/WebKit/chromium/public/OWNERS and Source/Platform/chromium/public/OWNERS, which tkent@ is in, are not changed in this patch so he's still an owner for public API.

Ah, I missed that. I thought those files were replaced with Source/WebKit/OWNERS. Thanks for clarifying

-jochen

[Adam Barth]

The way this works in the larger Chromium project is that it's up to the existing OWNERS.  For Core, because we have such a large set of OWNERS, it might make sense to discuss changes over email first, at least until we have more experience and a clearer set of norms.

Adam

 

https://codereview.chromium.org/14328010

Thanks,

Adam

[Jochen Eisinger]

On Wed, Apr 17, 2013 at 9:55 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:54 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 12:52 PM, Jochen Eisinger <joc...@chromium.org> wrote:

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

Source/WebKit/chromium/public/OWNERS and Source/Platform/chromium/public/OWNERS, which tkent@ is in, are not changed in this patch so he's still an owner for public API.

Ah, I missed that. I thought those files were replaced with Source/WebKit/OWNERS. Thanks for clarifying

Although Source/WebKit and Source/Platform are still large parts of blink that are being worked on a lot during the night (e.g. mediastream stuff)

best

-jochen

[James Robinson]

On Wed, Apr 17, 2013 at 1:03 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:55 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:54 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 12:52 PM, Jochen Eisinger <joc...@chromium.org> wrote:

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

Source/WebKit/chromium/public/OWNERS and Source/Platform/chromium/public/OWNERS, which tkent@ is in, are not changed in this patch so he's still an owner for public API.

Ah, I missed that. I thought those files were replaced with Source/WebKit/OWNERS. Thanks for clarifying

Although Source/WebKit and Source/Platform are still large parts of blink that are being worked on a lot during the night (e.g. mediastream stuff)

Source/Platform/ is essentially never touched outside of Source/Platform/chromium/public.

I would definitely support having a broader set of OWNERS for Source/WebKit/chromium/src and Source/WebKit/chromium/tests.

- James

[Adam Barth]

On Wed, Apr 17, 2013 at 1:05 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 1:03 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:55 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:54 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 12:52 PM, Jochen Eisinger <joc...@chromium.org> wrote:

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

Source/WebKit/chromium/public/OWNERS and Source/Platform/chromium/public/OWNERS, which tkent@ is in, are not changed in this patch so he's still an owner for public API.

Ah, I missed that. I thought those files were replaced with Source/WebKit/OWNERS. Thanks for clarifying

Although Source/WebKit and Source/Platform are still large parts of blink that are being worked on a lot during the night (e.g. mediastream stuff)

Source/Platform/ is essentially never touched outside of Source/Platform/chromium/public.

Yeah, there are only eight CPP files in that directory.  We should figure out what to do with them...  Maybe the best thing is to move them back into Core.

 

I would definitely support having a broader set of OWNERS for Source/WebKit/chromium/src and Source/WebKit/chromium/tests.

I've looked through the revision log and added some more people who appear to be active in reviewing CLs in those directories.

Thanks,

Adam

[Jochen Eisinger]

On Wed, Apr 17, 2013 at 10:21 PM, Adam Barth <aba...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 1:05 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 1:03 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:55 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 9:54 PM, James Robinson <jam...@chromium.org> wrote:

On Wed, Apr 17, 2013 at 12:52 PM, Jochen Eisinger <joc...@chromium.org> wrote:

I like the approach a lot, thanks for working on this.

One concern, however, is that we lose around the clock coverage for all of blink. Before, with tkent@ being a WebKit API owner, we could quickly iterate on all kind of changes during PST-night. With the proposed change, this wouldn't be the case anymore.

Source/WebKit/chromium/public/OWNERS and Source/Platform/chromium/public/OWNERS, which tkent@ is in, are not changed in this patch so he's still an owner for public API.

Ah, I missed that. I thought those files were replaced with Source/WebKit/OWNERS. Thanks for clarifying

Although Source/WebKit and Source/Platform are still large parts of blink that are being worked on a lot during the night (e.g. mediastream stuff)

Source/Platform/ is essentially never touched outside of Source/Platform/chromium/public.

Yeah, there are only eight CPP files in that directory.  We should figure out what to do with them...  Maybe the best thing is to move them back into Core.

 

I would definitely support having a broader set of OWNERS for Source/WebKit/chromium/src and Source/WebKit/chromium/tests.

I've looked through the revision log and added some more people who appear to be active in reviewing CLs in those directories.

Thank you!

[David Levin]

I take it back. Seems fine.

[Eric Seidel]

Correct me if I'm wrong, but one of the things that I really like
about the OWNERS system, is that it feels less like an "elite club"
and more like a "list of volunteer firefighters". I like that having
an OWNERS file next to code helps me know who understand the long-term
vision for that area of the code and can help me to do things right.

I'm sure we'll move lots of folks around on these lists, but I really
like this direction.

I would like to see us to continue to break core/ down into pieces and
develop (evolving) API layers between the pieces. Each of these
pieces could move to modules/ or the top-level and would get its own
OWNERS file. core/platform/image-decoders is one example of another
piece to slice off.

I think we should follow Chromium's model of OWNERS deciding who to
add to each local OWNERS file. In that way they are like "clubs", but
hopefully happy ones. :)

I think that core-owners, being so large, should probably get a
mailing list for communication.

[Ojan Vafai]

The patch lgtm.

On Wed, Apr 17, 2013 at 1:43 PM, Eric Seidel <ese...@chromium.org> wrote:

I would like to see us to continue to break core/ down into pieces and
develop (evolving) API layers between the pieces.  Each of these
pieces could move to modules/ or the top-level and would get its own
OWNERS file.  core/platform/image-decoders is one example of another
piece to slice off.

I think this is what your saying, but just to be clear, I'm not very excited about the idea of core/ having sub-OWNERS for each directory. If we are able to carve bits off and move them into modules/, it makes sense to me that each modules subdirectory would have it's own OWNERS file.

I think that core-owners, being so large, should probably get a
mailing list for communication.

Why is blink-dev insufficient?

[Eric Seidel]

On Wed, Apr 17, 2013 at 2:05 PM, Ojan Vafai <oj...@chromium.org> wrote:
> The patch lgtm.
>
> On Wed, Apr 17, 2013 at 1:43 PM, Eric Seidel <ese...@chromium.org> wrote:
>>
>> I would like to see us to continue to break core/ down into pieces and
>> develop (evolving) API layers between the pieces. Each of these
>> pieces could move to modules/ or the top-level and would get its own
>> OWNERS file. core/platform/image-decoders is one example of another
>> piece to slice off.
>
>
> I think this is what your saying, but just to be clear, I'm not very excited
> about the idea of core/ having sub-OWNERS for each directory. If we are able
> to carve bits off and move them into modules/, it makes sense to me that
> each modules subdirectory would have it's own OWNERS file.

Correct. I don't see core/ needing/wanting OWNERS files deeper than
the top level. But pieces which can be pulled out of core should. It
doesn't make sense to me to have core/foo/OWNERS as if "foo" is in
core/ it is likely deeply integrated with the rest of core and thus
likely that changes are going to need to span all of core/ including
core/foo/.

>> I think that core-owners, being so large, should probably get a
>> mailing list for communication.
>
>
> Why is blink-dev insufficient?

I'm OK to use blink-dev for now. It's not clear to me how the mailing
lists and OWNER lists will grow/change. if core/OWNERS grows to 100
people and blink-dev to several thousand, it's not clear that it will
still make sense to have core/OWNERS-only discussions on this list.

I presume you're suggesting that folks wishing to be added to
core/OWNERS would email blink-dev to propose their addition to
core/OWNERS and then need some number of LGTM's to be added?

Presumably we should start a separate thread about deciding that process?

[Takeshi Yoshino]

On Thu, Apr 18, 2013 at 6:10 AM, Eric Seidel <ese...@chromium.org> wrote:

> I think this is what your saying, but just to be clear, I'm not very excited
> about the idea of core/ having sub-OWNERS for each directory. If we are able
> to carve bits off and move them into modules/, it makes sense to me that
> each modules subdirectory would have it's own OWNERS file.

Correct.  I don't see core/ needing/wanting OWNERS files deeper than
the top level.  But pieces which can be pulled out of core should.  It
doesn't make sense to me to have core/foo/OWNERS as if "foo" is in
core/ it is likely deeply integrated with the rest of core and thus
likely that changes are going to need to span all of core/ including
core/foo/.

As discussed in the thread about the right place for XHR, Streams and Fetch [1], there could be directories we need to place in the core/ directory not because of deep integration with each other but due to dependency from multiple modules. For such a directory that also could be maintained comparatively independent from other stuff in core/, can't we consider adding a sub-OWNERS to the directory?

[1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GoFbe0yLO50/discussion