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
_______________________________________________
dev-tech-network mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-tech-network
