Re: netwerk/ code review policy [resend]

Sorry to let this sit....

I'm not sure a document with a formal review policy will make the 
process much easier, but I'm open to the idea.

There's a lot of potential areas of discussion here.  It might help to 
get more specific about what parts of our review process we want to 
re-examine and/or formalize.

- who's qualified to do a review?

The Firefox module has a more formal standard than we do about who's 
qualified to do a +r.  (level 3 commit access, plus voucher from 3 other 
reviewers, and this seems to map to "peer" in the Mozilla module owner 
sense, too).  We've been a lot looser with this, and generally just go 
with "this patch has been looked at by the best person to do the +r (or 
at least someone who knows the code well-enough)."   At times I've had 
people who aren't even on the necko team (ex: Josh Matthews) review 
patches, because they know the code and seemed like a good enough 
reviewer, especially given the workload of other potential reviewers.  
I'm personally OK with this process, given our module's size, and that 
it seems like it's worked alright to me.  But I'm open to suggestions.

- who ought to take a particular review

This can be a tricky issue, and involves both competence with the code 
area, and workload.   Josh (along with all of us in our bi-weekly calls) 
has been stepping in to wrangle matching patches with reviewers as our 
workload changes, and that strikes me as having been quite useful.  I'm 
not sure we can improve on that with rules in a document, but again, I'm 
open to ideas.

- how much can the patch author and the reviewer disagree?  What's a 
"fair" review of a patch, and what kind of things should remain 
implementer's choice and not be subject to a reviewer's "whims"?

This a very interesting issue.   The Firefox module document emphasizes 
politeness ("Saying *Thank you* for your patch"), and not being an 
arbitrary hard-liner about every detail (patch doesn't need to be "best 
possible solution that reviewers can envision", and "Style should be 
/consistent/, but it need not be /identical/.").   On the other hand, 
there's a *lot* of room in between "this patch does no harm" (to me, a 
fairly non-sensible metric:  I don't +r patches just because they do no 
harm) and "implements the best possible solution."  In practice, I think 
this comes down to the reviewer's judgment about whether a patch is 
adding enough value to be worth the inevitable risk it comes with, 
whether the design of the patch is good enough (at least for the moment: 
with other work left for followup bugs), and whether the implementation 
is tight enough that it's going to be as easy to maintain as we can 
achieve with reasonable effort.   Reviewers and patch authors can of 
course disagree on that :)   There are some rules of thumb we have:  a 
patch should generally have tests as part of the patch, instead of in a 
followup, because that vastly improves the chances that the test will 
actually be written and checked in promptly.  But there are exceptions 
even for that when it makes sense.

- Different reviewers seem to have different standards/opinions.  Can I 
switch reviewers if I don't like a review?

Another inevitable issue, at some level, though there may be areas where 
we can get more of a shared understanding about what kinds of standards 
we ought to have (are there any in particular you have in mind?).  The 
way we work on this reminds me a little of the US legal system:   you 
get an initial judge (often due in part to the luck of the draw), and 
then you stick with them, barring exceptional circumstances.  One reason 
for this--like in the law--is because there's overhead involved in 
getting a different reviewer up to speed.  For us, I think there's also 
an element of collegiality involved--it's not friendly to simply ditch a 
reviewer because you don't like their review.   In general, if someone 
who seems knowledgeable is opposed to a particular patch (this sometimes 
includes people who are neither on the necko team and nor even work for 
Mozilla--I'm thinking someone like Jo Hermans), we don't just ride over 
them, but instead widen the audience, and do whatever we need to to 
figure out the issue and get to consensus.  If that doesn't happen, 
there's always appeal to the module owner (or higher), but that's been 
pretty rare.  The only case I can think of was over whether to keep 
gopher in the stack or not (where Brendan wound up making the decision).

I guess this is a very long-winded way of saying I'm not sure we need a 
document like the Firefox module's.  I'd be open to approving something 
if someone else writes it up and it looks ok.   It might also work to 
get more specific about particular things in our process that seem 
broken and/or need clarification.

Jason



On 11/01/2011 07:57 AM, Patrick McManus wrote:
> (This is a resend because I didn't get a copy back and I normally do on
> this list. I think it might be because there were originally 2 addresses
> in the to: line . Apologies if it is a dup for you.)
>
> Hi Christian, necko-folk,
>
> Last week I read an update from the firefox module team about their code
> review policy in browser/* -
> https://wiki.mozilla.org/Firefox/Code_Review
>
> I like it very much because it addresses who the reviewers are, what
> they are responsible for, what a r+ means, and what is not required for
> a r+.
>
> All of these things have confused me about necko from time to time. ok,
> all the time if I'm being honest.
>
> I would like to propose we adopt a policy like the browser/* one above.
> I'd be happy with that exact policy actually but something else that
> clearly addresses the same points and that we can all get behind would
> be fine too.
>
> We are sort of de-facto doing some of these things - especially the
> who-are-the-reviewers bits. It would be good to write it down, but I'm
> most interested in defining what is and what isn't part of a review -
> that seems to be a more mercurial (ha!) definition.
>
> I'd love to hear from everyone, but especially Christian as module
> owner. What do you think?
>
> -Pat
>
>
> _______________________________________________
> dev-tech-network mailing list
> [email protected]
> https://lists.mozilla.org/listinfo/dev-tech-network