First step: mailnews & Thunderbird review guidelines
Dan Mosedale
In the interest of working towards better review throughput in the
mailnews & Thunderbird areas, I've put together a couple of draft pages
of review guidelines, based on the ones currently in use for Firefox and
Toolkit:
<http://www.mozilla.org/mailnews/review.html>
<http://www.mozilla.org/mailnews/review-mail.html>
These have not yet substantively changed the reviewers and modules much.
What they do do is provide policy guidelines and a jumping-off point
for splitting out more sub-modules as well as other cleanup.
I started with copies of the Firefox and Toolkit pages, modified for
policy differences, merged in the information from owners.html, and
split off a few new modules that effectively describe the way things are
already working today (Addressbook, IMAP, Message Database).
Feedback appreciated.
Next steps (to follow soon): splitting off more sub-modules, finding
peers for existing sub-modules, and pinging folks who haven't been
active for a while to sort out their ongoing roles.
Dan
Mitchell Baker
Dan
this seems to fit with current practices nicely.
Are you thinking of adding the new modules to owners.html?
Right now we're not tracking sub-modules in owners.html. That's
probably fine, I'm not sure how to do it and I'm not sure it's worth a
giant effort at this point -- it's easy to read in the wiki format -- as
long as people can find the relevant info.
mitchell
Mark Banner
> <http://www.mozilla.org/mailnews/review.html>
> <http://www.mozilla.org/mailnews/review-mail.html>
> Feedback appreciated.
A few thoughts/comments:
Both pages:
- Could we link to the super-reviewer list/page?
- The third point of unit-test rules, do you mean that the developer is
encouraged to write appropriate test code and *post it on the bug*? or
something similar?
MailNews page:
- "feelconfident" is two words not one ;-)
- In the individuals and roles section, the last sentence of the
paragraph contradicts the last bullet point of the Review Rules. This is
possibly as there seems to be some ambiguity on the page regarding
sub-module owners, reviewers and peers.
- General Unit Testing Sub-Module - by this, can you just clarify if you
mean just the support type of code, or all tests? Generally I'd like
tests to be reviewed alongside patches in their own areas, but where
we're doing things like fakeservers and defining new
architectures/styles to be involved with those.
Other comments:
- There is no mention of ui-review. Given we've brought this in
recently, I think we ought to at least mention it.
- Could we find a place for a central wiki page or something that can
list pointers to (super)review requirements for all projects based on
mozilla.org? and have a reverse link from those pages to that one?
Standard8
Karsten Düsterloh
> In the interest of working towards better review throughput in the
> mailnews & Thunderbird areas, I've put together a couple of draft pages
> of review guidelines, based on the ones currently in use for Firefox and
> Toolkit:
>
> <http://www.mozilla.org/mailnews/review.html>
| Movemail mailnews/movemail Philip K. Warren
So you did talk with him about it?
Karsten
Ian Neal
> Dan Mosedale wrote:
>> <http://www.mozilla.org/mailnews/review.html>
>> <http://www.mozilla.org/mailnews/review-mail.html>
>
>> Feedback appreciated.
>
> Other comments:
>
> - There is no mention of ui-review. Given we've brought this in
> recently, I think we ought to at least mention it.
> - Could we find a place for a central wiki page or something that can
> list pointers to (super)review requirements for all projects based on
> mozilla.org? and have a reverse link from those pages to that one?
>
> Standard8
Yes, I agree, need to mention ui-review and also ui frontend both shared
and forked. Not sure what is happening with help for TB...
IanN
Dan Mosedale
> this seems to fit with current practices nicely.
> Are you thinking of adding the new modules to owners.html?
> Right now we're not tracking sub-modules in owners.html. That's probably
> fine, I'm not sure how to do it and I'm not sure it's worth a giant
> effort at this point -- it's easy to read in the wiki format -- as long
> as people can find the relevant info.
That pretty much matches with my thinking. It would be nice to have
this all centrally maintained in one place (the fact that we've now got
four nearly identical pages is something of a red flag), so I can
imagine wanting a Despot v2 at some point in the future to deal with
this more generally.
Dan
Dan Mosedale
> Dan Mosedale wrote:
>> <http://www.mozilla.org/mailnews/review.html>
>> <http://www.mozilla.org/mailnews/review-mail.html>
>
>
> Both pages:
>
> - Could we link to the super-reviewer list/page?
Linked to from the MailNews page, no need on the Tb page, since Tb
doesn't require sr.
> - The third point of unit-test rules, do you mean that the developer is
> encouraged to write appropriate test code and *post it on the bug*? or
> something similar?
I borrowed that verbiage from the original toolkit doc, and I agree that
it's confusing. It's also not clear to me what exactly the best policy
is there. I'll make a note to ping bsmedberg to try and understand what
he meant and figure out the best plan here.
> MailNews page:
>
> - "feelconfident" is two words not one ;-)
Fixed.
> - In the individuals and roles section, the last sentence of the
> paragraph contradicts the last bullet point of the Review Rules. This is
> possibly as there seems to be some ambiguity on the page regarding
> sub-module owners, reviewers and peers.
I just killed that sentence. I don't think it really added much other
than confusion.
> - General Unit Testing Sub-Module - by this, can you just clarify if you
> mean just the support type of code, or all tests? Generally I'd like
> tests to be reviewed alongside patches in their own areas, but where
> we're doing things like fakeservers and defining new
> architectures/styles to be involved with those.
I meant what you're asking for. I changed the module name to "Unit
Testing Infrastructure" in the hopes that that's more clear.
> Other comments:
>
> - There is no mention of ui-review. Given we've brought this in
> recently, I think we ought to at least mention it.
I agree that we need to talk about this, but I'm trying to do this work
in small steps. It feel to me like we need to do some thinking about
how Thunderbird, SeaMonkey, and mailnews/ interact first, and this
doesn't feel as urgent as getting our sub-module infrastructure in
place, so I'd prefer not to entrain that discussion quite yet.
> - Could we find a place for a central wiki page or something that can
> list pointers to (super)review requirements for all projects based on
> mozilla.org? and have a reverse link from those pages to that one?
That's a good idea. I'd suggest filing a bug on that.
Dan
Dan Mosedale
> Dan Mosedale aber hob zu reden an und schrieb:
>> In the interest of working towards better review throughput in the
>> of review guidelines, based on the ones currently in use for Firefox and
>> Toolkit:
>>
>> <http://www.mozilla.org/mailnews/review.html>
>
> | Movemail mailnews/movemail Philip K. Warren
>
> So you did talk with him about it?
>
Not yet; that's what I was referring to by the next step of: