[webkit-dev] Why I'm reviewing patches outside my area (and why you should too)

[Adam Barth]

Over the past 24 hours, I've been aggressively reviewing patches in
pending-review <http://webkit.org/pending-review> that have been
sitting around for over a month.  My approach as been to review the
patches in order from oldest to newest with a "the buck stops here"
perspective.  That means I'm going to either r+ or r- the patch, even
if that means I have to spend several hours learning the related code.
 As of this morning, I've cleared out all the patches that have been
waiting for review for over a month.

Q: Why are you doing this?  You should stick to reviewing code in your own area.
A: The pending-review queue is out of control.  It's past the tipping
point where there are too many patches for someone to reasonably look
at the queue.  It's not healthy for the project to leave patches
unreviewed for over a month.  Someone needs to step up.  If not me,
then who?

Q: Those ancient patches are junk.  Why not focus on the newer
patches, which are higher quality and from more established
contributors?
A: Some of the ancient patches are junk, and I've been liberally r-ing
them with explanations, but a lot of them are actually quite good.
For example, there was a patch that fixed a NULL pointer dereference
that affected every port.  There was another patch that added a test
to an earlier version that was r+ed.  If we let patches like these
linger for a month, we're discouraging people from fixing crashes and
writing tests.

Q: What if you break things?
A: We can't be afraid of breaking things.  That will only paralyze the
project.  Instead, we should be happy when things break because it
shows us where we need more test coverage.

Q: Why are you reviewing editing patches, in particular?  You don't
know anything about editing.
A: For whatever reason, the folks who usually review editing patches
appear to be MIA.  That means we need to grow more editing expertise.
Personally, I have about zero interest in editing, but until Ojan is a
reviewer, someone needs to do it.  If you'd rather review editing
patches instead of me, please let me know and I'll happily forward the
reviews to you.

Q: What are the common reasons patches get stuck in pending-review and
who can we fix that?
A: Anecdotally, I've been seeing three main causes of patches getting stuck:
  (1) The patch needs to be reviewed by David Hyatt.   David Hyatt
appears to be a bottleneck in the project because he's an expert on a
number of components that no one else understands as well but he
doesn't spend as much time reviewing patches as Maciej or Darin.  I
think the best solution here is to have more folks gain expertise in
these areas.
  (2) The patch is for a port with fewer reviewers.  Reviews for ports
like BREW tend to collect in pending-review because there aren't that
many reviewers who are personally incentivized to review those
patches.  As those ports mature, this problem should resolve itself
naturally.  I've tried to help here where possible, but these patches
are the most difficult for me to review.
  (3) Someone reviewed an earlier version of the patch but then didn't
follow up.  I think having a partial review by one person encourages
other people to assume that person will finish the review.  That cause
the patch to float upwards in pending-review until it gets lost in the
sea of ancient patches.  I'd encourage reviewers to follow through
with their reviews.

Q: How long is this project taking you?
A: It seems to be taking me about an average of 45 minutes per patch.
That includes the time to read the entire bugzilla thread, review the
code, and read up on related areas of the code.  If you do the math,
you'll see that I won't be able to do this alone.  That's why I'm
asking for your help.

If you're a reviewer, I'd encourage you to work through pending-review
from oldest to newest.  Don't be afraid of r-ing a patch.  Believe me,
folks are thankful for feedback (even negative feedback) when their
patches have been sitting around collecting dust.

Adam