Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
On Tue, Mar 9, 2010 at 11:45 PM, Zoltan Herczeg zherc...@inf.u-szeged.huwrote: It's also a big help when peers (which aren't necessarily WebKit reviewers) look over it and give review-style feedback as well. Especially when said peers know more about that code than any of the official reviewers. Is that really help? Sometimes when a patch looks good to me, it still rots in the bugzilla for weeks. On the other hand, sometimes I have concerns about the patch, and somebody just pop in and give an r+ without any comments. It helps the patch submitter, and it helps reviewers who read comments. Sadly, if a reviewer doesn't read anything on the bug, there's not much we can do. One idea, I guess, would be to say it's OK for non-reviewers to r- a patch (for legit reasons), but only reviewers can r+ a patch. I'd be fine with this but I suspect it'd rub some people the wrong way. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
On Tue, Mar 9, 2010 at 1:17 PM, David Hyatt hy...@apple.com wrote: On Mar 9, 2010, at 1:45 PM, Adam Barth wrote: (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. Dave needs to review this is used as an excuse by others to get out of doing reviews in my opinion. :) MathML is a great example of this. I don't need to be the sole person reviewing MathML patches that don't affect core code. If the MathML rendering code lands in the tree with some mistakes or issues that can be fixed later, it's really not a big deal. Maybe I would have noticed something that another reviewer wouldn't have, but the mistakes will get caught eventually. It is a great example of this. While I've been very appreciative of Dave Hyatt's review and help, it has really been great to have others jump in a review my code. At this point, I've had more iterations of reviews and patches and the code (and my understanding of WebKit) has gotten quite a bit better just because people decided to review the MathML code rather than wait for it to get to the top of Dave's stack! -- --Alex Milowski The excellence of grammar as a guide is proportional to the paucity of the inflexions, i.e. to the degree of analysis effected by the language considered. Bertrand Russell in a footnote of Principles of Mathematics ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
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 webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
On Mar 9, 2010, at 1:45 PM, Adam Barth wrote: (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. Dave needs to review this is used as an excuse by others to get out of doing reviews in my opinion. :) MathML is a great example of this. I don't need to be the sole person reviewing MathML patches that don't affect core code. If the MathML rendering code lands in the tree with some mistakes or issues that can be fixed later, it's really not a big deal. Maybe I would have noticed something that another reviewer wouldn't have, but the mistakes will get caught eventually. dave (hy...@apple.com) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
(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 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. However as soon as you r- a patch, according to 3, you need to finish the review when it gets re-submitted. This leads me to believe that *one shouldn't avoid a patch that has feedback from someone else* unless one doesn't feel qualified to judge whether the feedback has been addressed. I plea to folks submitting patches: 1. Keep your patches as small as possible. It is no fun to deal with a 60K patch. 2. Review your own patches before or right after you attach them to the bug as if you were reviewing someone else's code. 3. Address any style issues, build issues that the bots notice. You don't need to wait for a review to point out the same issue. dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
On Tue, Mar 9, 2010 at 9:39 PM, David Levin le...@chromium.org wrote: (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 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. However as soon as you r- a patch, according to 3, you need to finish the review when it gets re-submitted. This leads me to believe that *one shouldn't avoid a patch that has feedback from someone else* unless one doesn't feel qualified to judge whether the feedback has been addressed. I plea to folks submitting patches: 1. Keep your patches as small as possible. It is no fun to deal with a 60K patch. 2. Review your own patches before or right after you attach them to the bug as if you were reviewing someone else's code. 3. Address any style issues, build issues that the bots notice. You don't need to wait for a review to point out the same issue. It's also a big help when peers (which aren't necessarily WebKit reviewers) look over it and give review-style feedback as well. Especially when said peers know more about that code than any of the official reviewers. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
On Tue, Mar 9, 2010 at 11:45 AM, Adam Barth aba...@webkit.org wrote: 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? snip 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. Also, it's OK to comment on a patch with a suggestion as to how to make it more easily reviewable. As an example, Eric noted on this patch of mine ( https://bugs.webkit.org/show_bug.cgi?id=35314) that most of the layout test changes could be committed as a separate patch before making any code changes and that would make verifying correctness of the code changes easier. While it was probably more work for me, I appreciated the opportunity to make progress instead of just stalling. Ojan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why I'm reviewing patches outside my area (and why you should too)
Hi, It's also a big help when peers (which aren't necessarily WebKit reviewers) look over it and give review-style feedback as well. Especially when said peers know more about that code than any of the official reviewers. Is that really help? Sometimes when a patch looks good to me, it still rots in the bugzilla for weeks. On the other hand, sometimes I have concerns about the patch, and somebody just pop in and give an r+ without any comments. Zoltan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
