Proposed Changes to Toolkit ownership structure and reviews

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: Proposed Changes to Toolkit ownership structure and reviews

Mike Connor-3
Axel Hecht wrote:

> <..>
>
>> Locales
>>     toolkit/locales (build/packaging/policy stuff, string approvals
>> are ui-review fodder)
>>     Benjamin Smedberg
>>     Axel Hecht
>
> I'd rather have that the other way around, and I'm not sure what you
> think is "ui-review fodder".
>
> Axel

ui-review is for approving the actual new/changed strings.

Given that bsmedberg is the person who implemented most (all?) of the
locale structure and packaging-fu, I think he's the appropriate owner.

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposed Changes to Toolkit ownership structure and reviews

Axel Hecht
Mike Connor wrote:

> Axel Hecht wrote:
>> <..>
>>
>>> Locales
>>>     toolkit/locales (build/packaging/policy stuff, string approvals
>>> are ui-review fodder)
>>>     Benjamin Smedberg
>>>     Axel Hecht
>>
>> I'd rather have that the other way around, and I'm not sure what you
>> think is "ui-review fodder".
>>
>> Axel
>
> ui-review is for approving the actual new/changed strings.
>
> Given that bsmedberg is the person who implemented most (all?) of the
> locale structure and packaging-fu, I think he's the appropriate owner.

That concept failed recently.

I definitely need to be in the loop of any of these. Not sure why you
would need an owner separately from that.

Axel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposed Changes to Toolkit ownership structure and reviews

Mike Connor-4

On 4-Apr-06, at 2:17 PM, Axel Hecht wrote:

> Mike Connor wrote:
>> Axel Hecht wrote:
>>> <..>
>>>
>>>> Locales
>>>>     toolkit/locales (build/packaging/policy stuff, string  
>>>> approvals are ui-review fodder)
>>>>     Benjamin Smedberg
>>>>     Axel Hecht
>>>
>>> I'd rather have that the other way around, and I'm not sure what  
>>> you think is "ui-review fodder".
>>>
>>> Axel
>> ui-review is for approving the actual new/changed strings.
>> Given that bsmedberg is the person who implemented most (all?) of  
>> the locale structure and packaging-fu, I think he's the  
>> appropriate owner.
>
> That concept failed recently.

What failed recently?  Changes to strings themselves are for ui-
review, but the technical review should catch unchanged entities,  
etc, if that's what you're talking about.

> I definitely need to be in the loop of any of these. Not sure why  
> you would need an owner separately from that.

On any of what?  String changes? I can make it so you see bugmail on  
all ui-review requests, but this is about code ownership, not being-
in-the-loop.  The structure and layout of how we build locales is  
what bsmedberg would own, which isn't what you're really talking about.

What problem are you trying to solve?

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposed Changes to Toolkit ownership structure and reviews

Mike Connor-4
In reply to this post by Mike Connor-4
Aside from Axel's concerns, which I'm responding to separately (and those concerns are very localized, pardon the pun), were there any other issues to be raised before I start making despot and website changes?  I was hoping to wrap those tasks up over the weekend, so feedback would be great, and I would encourage anyone with concerns they don't want to air publicly to mail me directly.

Thanks to everyone who helped shape the list, especially darin!

-- Mike

On 27-Mar-06, at 10:06 AM, Mike Connor wrote:

As a little background, there's been some private discussion about rejigging the way we break down ownership of the various parts of toolkit.  Ben asked me to take the detailed proposal public, so here goes:

Reviews getting to the right place often happens only because people are told who to ask by active community members.  There is no obvious way to find out by yourself who is the best person to ask for review, or who to ask for help in figuring out where to fix a bug, etc.  I believe in the principle of strong ownership, and with the size and breadth of toolkit its hard for any one person to be that owner.  Breaking things up into sub-modules should mean that there's a clear advocate and strong hacker acting as a strong owner for all of the code we're relying on.  The change in how we're defining members and how they gain responsibilities should help more people get involved and feel like they're being recognized, since its easier to establish a good track record with a smaller group before branching out.

This isn't absolutely necessary yet, with the current group of hackers, but I believe that adding the structure now will provide a better framework for adding new people to the group going forward.  I want to create a clearer progression for new people getting involved in FE hacking (get involved, get responsibility for a small area and start helping with reviews, branch out into wider areas and eventually become a peer for the whole toolkit) which reflects the module peer->owner->superreviewer progression that seems to have worked for Gecko in many cases.  It also allows people to not branch out, and just focus on the areas they're interested in working on, and still be given appropriate responsibility.  Its a bit more of a benevolent feudal system than our traditional benevolent dictatorship, but I think its time to start building a larger hierarchy of people so there's more perceived room to fit in.

The more I look at how things are being done, and who is taking the leadership roles, I'd like to propose Benjamin Smedberg as the overall toolkit owner.  Both on a technical level and with his focus on Mozilla-as-platform, he's obviously well-qualified, and he is doing the most to ensure that the toolkit is a strong platform.  This also means that there's a check and balance between the needs of the apps, and the needs of other apps not reflected in the current structure (Songbird, Nvu, Sunbird, etc).

I also propose that Rob Strong should be elevated from member to peer, and Neil Deakin should be added as a peer.

Proposed Toolkit ownership structure (bold means changes in despot)


Toolkit Technical Lead: Benjamin Smedberg (listed as owner in despot)
UI Group: Ben Goodger, Mike Beltzner, Mike Connor
Peers (shown in despot): Vladimir Vukicevic, Scott MacGregor, Neil Rashbrook, Michael Connor, Brian Ryner, Darin Fisher, Ben Goodger, Robert Strong, Neil Deakin
Members: Asaf Romano, Masayuki Nagano, Brett Wilson

Toolkit Sub-Modules

Sub-Module
Code covered
Owner
Other Reviewers
Toolkit bootstrapping and startuptoolkit/xre, toolkit/library, toolkit/profile, toolkit/components/startup, toolkit/components/commandlines, toolkit/components/remote, toolkit/components/build
Benjamin Smedberg Darin Fisher
Toolkit general UI bits
toolkit/content, toolkit/components (for those not covered elsewhere)
Mike Connor
Neil Deakin, Neil Rashbrook, Asaf Romano
Extension Manager
toolkit/mozapps/extensions
Rob Strong
Benjamin Smedberg, Mike Connor, Ben Goodger
Installer
toolkit/mozapps/installer (and later, wherever NSIS lives)
Rob Strong
Benjamin Smedberg
Find Toolbar
toolkit/components/typeaheadfind
Mike Connor
Masayuki Nagano
Locales
toolkit/locales (build/packaging/policy stuff, string approvals are ui-review fodder)
Benjamin Smedberg
Axel Hecht
Prefwindow
toolkit/content/widgets/preferences.xml, toolkit/mozapps/preferences
Mike Connor
Ben Goodger, Asaf Romano
Download Manager
toolkit/components/downloads, toolkit/mozapps/downloads
Ben Goodger
Mike Connor, Neil Deakin
Password Manager/Satchel
toolkit/components/passwordmgr, toolkit/components/satchel,
Brian Ryner

Software Update
toolkit/mozapps/update
Darin Fisher
Ben Goodger, Benjamin Smedberg, Rob Strong
GNOME Integration bits
toolkit/components/gnome
Brian Ryner
Darin Fisher
Chrome Registry
chrome
Benjamin Smedberg
Darin Fisher, Neil Rashbrook
Storage
storage
Vladimir VukicevicDarin Fisher, Brett Wilson, Brian Ryner
XULRunner
xulrunner
Benjamin Smedberg
Rob Strong, Darin Fisher

Review Rules:

Wherever possible, significant patches in a certain area should be reviewed by one of the listed sub-module owner/reviewers.
Peers may grant review in any area covered by toolkit, provided they feel confident in their knowledge of the code being changed.
One review is sufficient in most cases.  If the first reviewer feels that the patch would benefit from additional reviews, they should request a second review from an appropriate person.
Members may only grant review in areas they are listed as reviewers  (Author's note: this is to allow us to bring new people into certain areas without immediately giving them wide-ranging responsibility)
Significant UI changes should get UI review from someone in the UI group, along with code review.

Thoughts and feedback appreciated!

-- Mike
_______________________________________________
dev-planning mailing list


_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposed Changes to Toolkit ownership structure and reviews

Axel Hecht
In reply to this post by Axel Hecht
Mike Connor wrote:

>
> On 4-Apr-06, at 2:17 PM, Axel Hecht wrote:
>
>> Mike Connor wrote:
>>> Axel Hecht wrote:
>>>> <..>
>>>>
>>>>> Locales
>>>>>     toolkit/locales (build/packaging/policy stuff, string approvals
>>>>> are ui-review fodder)
>>>>>     Benjamin Smedberg
>>>>>     Axel Hecht
>>>>
>>>> I'd rather have that the other way around, and I'm not sure what you
>>>> think is "ui-review fodder".
>>>>
>>>> Axel
>>> ui-review is for approving the actual new/changed strings.
>>> Given that bsmedberg is the person who implemented most (all?) of the
>>> locale structure and packaging-fu, I think he's the appropriate owner.
>>
>> That concept failed recently.
>
> What failed recently?  Changes to strings themselves are for ui-review,
> but the technical review should catch unchanged entities, etc, if that's
> what you're talking about.

I'm talking about architecture questions here.
See https://bugzilla.mozilla.org/show_bug.cgi?id=320504 and
https://bugzilla.mozilla.org/show_bug.cgi?id=327465 for reference.

If we ever localize the uninstaller, this mistake is going to haunt us
again, I'm sadly sure about that.
That change did have bsmedberg's OK, and it broke the l10n checks he
wrote himself.
And I was the one who had to fight for backing this stuff out, or at
least move it out of l10n, so as things stand, I own that part these days.

The other mishap like this was caught early (#ifdef MOZ_PLACES inside a
DTD) and got raised by a localizer to me and was handled by me (didn't
require more than a bug comment, but still).

>> I definitely need to be in the loop of any of these. Not sure why you
>> would need an owner separately from that.
>
> On any of what?  String changes? I can make it so you see bugmail on all
> ui-review requests, but this is about code ownership, not
> being-in-the-loop.  The structure and layout of how we build locales is
> what bsmedberg would own, which isn't what you're really talking about.
>
> What problem are you trying to solve?
>

Benjamin wrote the code, yes, but I'm not convinced that he wants to own
it these days.

Axel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
12