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 _______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
