Re: [webkit-dev] review queue crazy idea
On Jul 23, 2010, at 1:10 PM, Dirk Pranke wrote: I have been thinking along these lines as well. I'm not sure how relevant touching existing lines of code is versus just other people who have hacked on the file at all or who have hacked on other files in the same directory (i.e., you'd need to address new code and new files, too). I think some empirical testing and tweaking would be the way to evaluate this, though. To find a reviewer, what you want to find is people who understand the relevant subsystems well, and perhaps also people who are good reviewers in general (have a high likelihood of spotting avoidable problems). Making lots of commits to (or touching lots of lines in) the same file or the same directory are at best proxies for that kind of information. They may be better proxies for that kind of information than self-identification, but that has yet to be demonstrated. While an algorithm is a good starting point, I think self-identification and/or peer identification can capture nuances that an algorithm will not. I think the main problems with http://trac.webkit.org/wiki/WebKit%20Team are that (a) people don't know to look there; and (b) people don't know or don't bother to update it. I don't think the accuracy of the information is a problem (other than possibly being out of date). I don't see how it is helpful to refer to this information as bragging. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Sat, Jul 24, 2010 at 4:09 PM, Maciej Stachowiak m...@apple.com wrote: I think the main problems with http://trac.webkit.org/wiki/WebKit%20Team are that (a) people don't know to look there; and (b) people don't know or don't bother to update it. I totally agree. I'll also add that the information on WebKit Team is too broad. For example, one can say that he's an expert in HTML editing but he might not be too familiar with how TextIterator advances or how ApplyStyleCommand pushes down text decorations. I'm sure there are similar problems with other subsystems as well. Aggregating information out of the svn blame will solve this problem as well. I'd also recommend to provide a way to list committers who recently touched that code as well because they are also likely to be able to find problems with new patches. Best, Ryosuke Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
I've never really liked trac.webkit.org/wiki/WebKit%20Team. Its always seemed more of place to brag about webkit involvement, than a useful reference. I think we could build a much better who should I ask to review this tool based on SVN information. -eric On Fri, Jul 23, 2010 at 12:15 AM, David Kilzer ddkil...@webkit.org wrote: We should also publicize/update these existing resources to help patch authors find reviewers for their patches: http://trac.webkit.org/wiki/CodeReview http://trac.webkit.org/wiki/WebKit%20Team I think the most effective approach is when patch authors proactively seek out reviewers. We're all busy, but when I'm asked to review a patch, I make time for it or point the person at another reviewer. Dave -- Sent from my iPhone 4 On Jul 22, 2010, at 12:29 AM, Maciej Stachowiak m...@apple.com wrote: On Jul 21, 2010, at 3:41 PM, Eric Seidel wrote: Wow. I really like this idea of helping contributors better understand what's going wrong. But, I think that even better would be to build a better front-end for reviews. Or a bot which knew how to suggest reviewers (based on annotate information from lines changed). I think a better UI for reviews, plus some better attempts at active notification of potential reviewers, could go a long way. I'm a strong believer in trying nudges and positive incentives before implementing harsher policies. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Fri, Jul 23, 2010 at 12:51 PM, Eric Seidel e...@webkit.org wrote: I've never really liked trac.webkit.org/wiki/WebKit%20Team. Its always seemed more of place to brag about webkit involvement, than a useful reference. I think we could build a much better who should I ask to review this tool based on SVN information. Were you thinking of some kind of automated harvesting of this information? I seems like a simple data file that can be processed by systems would be a good way to start. Then people can submit patches (or commit changes) to file changing the areas that they are willing to review and others can see/review that commitment. People who are looking for a reviewer can look through that file for individuals. If a review doesn't want to get reminder e-mails or requests from systems or individuals, they would then have to remove that review area for themselves from that data file. I would personally use an XML format ... but that's me. ;) -- --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
Re: [webkit-dev] review queue crazy idea
Given a patch file, you have its line number ranges. Given a git checkout, you can very quickly find who has made changes to what lines in that file. You then can have a bot post to the bug, saying that 10 people have touched the lines you're touching in your patch. 3 of them are active reviewers, here are their names: You could even educate such a script/bot about whitespace or rename changes so they're excluded from any who touched this lookup. I would like to build (or see built) this (or a similar) script. I just haven't had the time to do it. It's better than manual owner files or watch lists IMO because it's automatically generated. -eric On Fri, Jul 23, 2010 at 8:04 AM, Alex Milowski a...@milowski.org wrote: On Fri, Jul 23, 2010 at 12:51 PM, Eric Seidel e...@webkit.org wrote: I've never really liked trac.webkit.org/wiki/WebKit%20Team. Its always seemed more of place to brag about webkit involvement, than a useful reference. I think we could build a much better who should I ask to review this tool based on SVN information. Were you thinking of some kind of automated harvesting of this information? I seems like a simple data file that can be processed by systems would be a good way to start. Then people can submit patches (or commit changes) to file changing the areas that they are willing to review and others can see/review that commitment. People who are looking for a reviewer can look through that file for individuals. If a review doesn't want to get reminder e-mails or requests from systems or individuals, they would then have to remove that review area for themselves from that data file. I would personally use an XML format ... but that's me. ;) -- --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 mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Fri, Jul 23, 2010 at 1:11 PM, Eric Seidel e...@webkit.org wrote: Given a patch file, you have its line number ranges. Given a git checkout, you can very quickly find who has made changes to what lines in that file. You then can have a bot post to the bug, saying that 10 people have touched the lines you're touching in your patch. 3 of them are active reviewers, here are their names: That sounds like a heat map for code. I wonder if there are existing tools that do that? -- --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
Re: [webkit-dev] review queue crazy idea
On Fri, Jul 23, 2010 at 1:15 PM, Alex Milowski a...@milowski.org wrote: On Fri, Jul 23, 2010 at 1:11 PM, Eric Seidel e...@webkit.org wrote: Given a patch file, you have its line number ranges. Given a git checkout, you can very quickly find who has made changes to what lines in that file. You then can have a bot post to the bug, saying that 10 people have touched the lines you're touching in your patch. 3 of them are active reviewers, here are their names: That sounds like a heat map for code. I wonder if there are existing tools that do that? This looks interesting: http://www.statsvn.org/ I'm not sure if it can answer this line of code has these reviewers but it is worth a look. -- --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
Re: [webkit-dev] review queue crazy idea
I have been thinking along these lines as well. I'm not sure how relevant touching existing lines of code is versus just other people who have hacked on the file at all or who have hacked on other files in the same directory (i.e., you'd need to address new code and new files, too). I think some empirical testing and tweaking would be the way to evaluate this, though. -- Dirk On Fri, Jul 23, 2010 at 5:11 AM, Eric Seidel e...@webkit.org wrote: Given a patch file, you have its line number ranges. Given a git checkout, you can very quickly find who has made changes to what lines in that file. You then can have a bot post to the bug, saying that 10 people have touched the lines you're touching in your patch. 3 of them are active reviewers, here are their names: You could even educate such a script/bot about whitespace or rename changes so they're excluded from any who touched this lookup. I would like to build (or see built) this (or a similar) script. I just haven't had the time to do it. It's better than manual owner files or watch lists IMO because it's automatically generated. -eric On Fri, Jul 23, 2010 at 8:04 AM, Alex Milowski a...@milowski.org wrote: On Fri, Jul 23, 2010 at 12:51 PM, Eric Seidel e...@webkit.org wrote: I've never really liked trac.webkit.org/wiki/WebKit%20Team. Its always seemed more of place to brag about webkit involvement, than a useful reference. I think we could build a much better who should I ask to review this tool based on SVN information. Were you thinking of some kind of automated harvesting of this information? I seems like a simple data file that can be processed by systems would be a good way to start. Then people can submit patches (or commit changes) to file changing the areas that they are willing to review and others can see/review that commitment. People who are looking for a reviewer can look through that file for individuals. If a review doesn't want to get reminder e-mails or requests from systems or individuals, they would then have to remove that review area for themselves from that data file. I would personally use an XML format ... but that's me. ;) -- --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 mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
Patches sit in the queue for numerous reasons. Some of us used to scan the queue every day. But I've stopped doing that. Now I scan it more like once a week or two. There is no good way to find which patches might I have a chance of reviewing, so you end up spending 30 minutes just to find a patch you could actually review. You mean basically the problem is how a reviewer find a patch which he/she can review? WebKit as a project does not encourage matching a patch to a reviewer, but those, who are here for a long time know who can review their patches, and just CC them to the bug. This is difficult for a new contributor. I suggest a keyword - reviewer matching bot: a contributor is encouraged to select some keywords related to his/her patch from a list, and a bot could automatically CC some reviewers based on these keywords and other attributes (like component) of the bug report. Most patches get rejected for easily-bot-detectable reasons. Like bad or missing ChangeLogs, poor comment style, tabs, breaking EWS bots. Now that the style bot and EWS bots work better we should at least cq- patches which fail those bots (or fail to apply). The bots set the boxes to red near the patch, and post a small message to the bug report. This is more than enough for me, but a new contributor might have no idea how to fix them. A link to a wiki page should be added to these posts, which explains the steps to fix such problems. Or just mention some names, whose are an expert of different build systems, and can help to fix build issues. Zoltan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Jul 21, 2010, at 3:41 PM, Eric Seidel wrote: Wow. I really like this idea of helping contributors better understand what's going wrong. But, I think that even better would be to build a better front-end for reviews. Or a bot which knew how to suggest reviewers (based on annotate information from lines changed). I think a better UI for reviews, plus some better attempts at active notification of potential reviewers, could go a long way. I'm a strong believer in trying nudges and positive incentives before implementing harsher policies. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Thu, Jul 22, 2010 at 8:29 AM, Maciej Stachowiak m...@apple.com wrote: I think a better UI for reviews, plus some better attempts at active notification of potential reviewers, could go a long way. I'm a strong believer in trying nudges and positive incentives before implementing harsher policies. Maybe we could have the system try to contact reviewers who have expressed interest/ability in review patches for certain areas (or components)? For example, if someone registered as being a reviewer for MathML patches, the system could e-mail all the registered reviewers. If the set of reviewers is too large, the system could pick a random subset to e-mail. I have certainly had a consistent set of individuals who have been kind enough to review my patches. On occasion, I've contacted individuals to ask directly for a review when a patch has been there for awhile. I've gotten comfortable enough with the process to do this. Newer contributors or those contributing outside their normal areas might not feel so comfortable and so having the system do this would be very good for keeping things moving forward. -- --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
Re: [webkit-dev] review queue crazy idea
We should also publicize/update these existing resources to help patch authors find reviewers for their patches: http://trac.webkit.org/wiki/CodeReview http://trac.webkit.org/wiki/WebKit%20Team I think the most effective approach is when patch authors proactively seek out reviewers. We're all busy, but when I'm asked to review a patch, I make time for it or point the person at another reviewer. Dave -- Sent from my iPhone 4 On Jul 22, 2010, at 12:29 AM, Maciej Stachowiak m...@apple.com wrote: On Jul 21, 2010, at 3:41 PM, Eric Seidel wrote: Wow. I really like this idea of helping contributors better understand what's going wrong. But, I think that even better would be to build a better front-end for reviews. Or a bot which knew how to suggest reviewers (based on annotate information from lines changed). I think a better UI for reviews, plus some better attempts at active notification of potential reviewers, could go a long way. I'm a strong believer in trying nudges and positive incentives before implementing harsher policies. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] review queue crazy idea
There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. For people new-ish to the WebKit project, it is often confusing both degree of responsibility that lies with the contributor to make the patch easy to review and the need to get reviewers' attention for a given patch. This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. On the other hand, knowing that patches will magically fall off the end of the queue might encourage reviewers to just ignore some patches. An alternative to auto-rejecting patches would be to send a nag email once a week to webkit-reviewers@ with the list of patches that are over a month old. Here are my initial thoughts on what a review bot would do. *After a patch turns a week old, send the following email:* Patch 12345 of bug 6789 is a week old. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is three weeks old:* Patch 12345 of bug 6789 is three weeks old. If it is still unreviewed in a week, it will automatically be rejected. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is a month old:* Patch 12345 of bug 6789 has been rejected because it is too old. This is likely because no webkit reviewer has been able to review it. If you would still like the patch reviewed, then please do the following: 1. Make sure your patch still applies to tip of tree. 2. Do as many of the suggestions at http://trac.webkit.org/wiki/CodeReview as possible. 3. Upload your patch for review again. -Webkit Review Bot ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Wed, Jul 21, 2010 at 2:40 PM, Ojan Vafai o...@chromium.org wrote: Here are my initial thoughts on what a review bot would do. *After a patch turns a week old, send the following email:* Patch 12345 of bug 6789 is a week old. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is three weeks old:* Patch 12345 of bug 6789 is three weeks old. If it is still unreviewed in a week, it will automatically be rejected. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is a month old:* Patch 12345 of bug 6789 has been rejected because it is too old. This is likely because no webkit reviewer has been able to review it. If you would still like the patch reviewed, then please do the following: 1. Make sure your patch still applies to tip of tree. 2. Do as many of the suggestions at http://trac.webkit.org/wiki/CodeReview as possible. 3. Upload your patch for review again. If we want to implement this, I think there ought to be some sort of easy way for a patch author to respond to any of these automated actions with some kind of a I looked at the suggestions, my patch is as easy-to-review as possible, please don't close it and instead help me flag down some reviewers action. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Jul 21, 2010, at 2:40 PM, Ojan Vafai wrote: There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. For people new-ish to the WebKit project, it is often confusing both degree of responsibility that lies with the contributor to make the patch easy to review and the need to get reviewers' attention for a given patch. This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. On the other hand, knowing that patches will magically fall off the end of the queue might encourage reviewers to just ignore some patches. An alternative to auto-rejecting patches would be to send a nag email once a week to webkit-reviewers@ with the list of patches that are over a month old. I think we should try the nag email first. I like the idea of advice on how to get a review. I think automatic rejection is kind of unfriendly, so I'd like to try other steps first. - Maciej Here are my initial thoughts on what a review bot would do. After a patch turns a week old, send the following email: Patch 12345 of bug 6789 is a week old. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot After the patch is three weeks old: Patch 12345 of bug 6789 is three weeks old. If it is still unreviewed in a week, it will automatically be rejected. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot After the patch is a month old: Patch 12345 of bug 6789 has been rejected because it is too old. This is likely because no webkit reviewer has been able to review it. If you would still like the patch reviewed, then please do the following: Make sure your patch still applies to tip of tree. Do as many of the suggestions at http://trac.webkit.org/wiki/CodeReview as possible. Upload your patch for review again. -Webkit Review Bot ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
Wow. I really like this idea of helping contributors better understand what's going wrong. But, I think that even better would be to build a better front-end for reviews. Or a bot which knew how to suggest reviewers (based on annotate information from lines changed). I encourage you to write any sort of better review tool/bot, turn it on, and see what happens. That's kinda what we did with the EWS and commit-queue. Initial reactions (to both) were strongly negative, but we fixed a bunch of stuff from initial feedback, and I (would like to) believe we have two useful systems now. I see the same pattern happening for someone trying to build some better review tools. -eric On Wed, Jul 21, 2010 at 5:40 PM, Ojan Vafai o...@chromium.org wrote: There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. For people new-ish to the WebKit project, it is often confusing both degree of responsibility that lies with the contributor to make the patch easy to review and the need to get reviewers' attention for a given patch. This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. On the other hand, knowing that patches will magically fall off the end of the queue might encourage reviewers to just ignore some patches. An alternative to auto-rejecting patches would be to send a nag email once a week to webkit-reviewers@ with the list of patches that are over a month old. Here are my initial thoughts on what a review bot would do. After a patch turns a week old, send the following email: Patch 12345 of bug 6789 is a week old. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot After the patch is three weeks old: Patch 12345 of bug 6789 is three weeks old. If it is still unreviewed in a week, it will automatically be rejected. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot After the patch is a month old: Patch 12345 of bug 6789 has been rejected because it is too old. This is likely because no webkit reviewer has been able to review it. If you would still like the patch reviewed, then please do the following: Make sure your patch still applies to tip of tree. Do as many of the suggestions at http://trac.webkit.org/wiki/CodeReview as possible. Upload your patch for review again. -Webkit Review Bot ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
Hey, I just don't understand how can the patches can sit in bugzilla unreviewed for weeks? There are 76 reviewers in the trac's team list. I think the reviewers have to do what they have assumed... Reviewing patches! I agree with Maciej automatic rejection is unfriendly. (Of course we can reject patches which are no longer applies on trunk. Yes, we should do this!) I think we should find a good way to advise the reviewers of the patch's topic. I prefer this way of automation. Regards, Zoltan On Wednesday 21 July 2010, at 23:40, Ojan Vafai wrote: There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. For people new-ish to the WebKit project, it is often confusing both degree of responsibility that lies with the contributor to make the patch easy to review and the need to get reviewers' attention for a given patch. This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. On the other hand, knowing that patches will magically fall off the end of the queue might encourage reviewers to just ignore some patches. An alternative to auto-rejecting patches would be to send a nag email once a week to webkit-reviewers@ with the list of patches that are over a month old. Here are my initial thoughts on what a review bot would do. *After a patch turns a week old, send the following email:* Patch 12345 of bug 6789 is a week old. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is three weeks old:* Patch 12345 of bug 6789 is three weeks old. If it is still unreviewed in a week, it will automatically be rejected. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is a month old:* Patch 12345 of bug 6789 has been rejected because it is too old. This is likely because no webkit reviewer has been able to review it. If you would still like the patch reviewed, then please do the following: 1. Make sure your patch still applies to tip of tree. 2. Do as many of the suggestions at http://trac.webkit.org/wiki/CodeReview as possible. 3. Upload your patch for review again. -Webkit Review Bot ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
Patches sit in the queue for numerous reasons. Some of us used to scan the queue every day. But I've stopped doing that. Now I scan it more like once a week or two. There is no good way to find which patches might I have a chance of reviewing, so you end up spending 30 minutes just to find a patch you could actually review. Most patches get rejected for easily-bot-detectable reasons. Like bad or missing ChangeLogs, poor comment style, tabs, breaking EWS bots. Now that the style bot and EWS bots work better we should at least cq- patches which fail those bots (or fail to apply). I think reviewing would be much easier if we had some better site by which to review. I'm not sure how we solve the social problem of getting people to review patches which didn't come from the person sitting next to them in their office however. Then again, that's sorta OK. part of contributing to webkit is integrating into the community. It's importnat to know your reviewers and to discuss things with them in channels other than the bug. But I still think some minimal technical work would go a long way to making reviewing better. -eric On Wed, Jul 21, 2010 at 7:34 PM, Zoltan Horvath zol...@webkit.org wrote: Hey, I just don't understand how can the patches can sit in bugzilla unreviewed for weeks? There are 76 reviewers in the trac's team list. I think the reviewers have to do what they have assumed... Reviewing patches! I agree with Maciej automatic rejection is unfriendly. (Of course we can reject patches which are no longer applies on trunk. Yes, we should do this!) I think we should find a good way to advise the reviewers of the patch's topic. I prefer this way of automation. Regards, Zoltan On Wednesday 21 July 2010, at 23:40, Ojan Vafai wrote: There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. For people new-ish to the WebKit project, it is often confusing both degree of responsibility that lies with the contributor to make the patch easy to review and the need to get reviewers' attention for a given patch. This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. On the other hand, knowing that patches will magically fall off the end of the queue might encourage reviewers to just ignore some patches. An alternative to auto-rejecting patches would be to send a nag email once a week to webkit-reviewers@ with the list of patches that are over a month old. Here are my initial thoughts on what a review bot would do. *After a patch turns a week old, send the following email:* Patch 12345 of bug 6789 is a week old. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is three weeks old:* Patch 12345 of bug 6789 is three weeks old. If it is still unreviewed in a week, it will automatically be rejected. It may just be because no reviewer has found time to review it. But there may be steps you can take to help get your patch reviewed. See http://trac.webkit.org/wiki/CodeReview for a few suggestions. -WebKit review bot *After the patch is a month old:* Patch 12345 of bug 6789 has been rejected because it is too old. This is likely because no webkit reviewer has been able to review it. If you would still like the patch reviewed, then please do the following: 1. Make sure your patch still applies to tip of tree. 2. Do as many of the suggestions at http://trac.webkit.org/wiki/CodeReview as possible. 3. Upload your patch for review again. -Webkit Review Bot ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
On Wed, Jul 21, 2010 at 2:40 PM, Ojan Vafai o...@chromium.org wrote: There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. I'd love to see a bot that educates people but I'm not sure if we should auto-rejects patches that are simply old. Maybe we need a little bit more of graduality like pinging the author of the patch first, and then make it obsolete if nobody responds etc... This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. I'd agree that having a really large review queue isn't ideal. Maybe we can customize the review queue so that it only shows patches of your expertise. Best, Ryosuke Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] review queue crazy idea
I've had patches sit in the review queue for 4 weeks then receive a positive review and land without much incident. Some patches are difficult to review or have a limited number of potential reviewers. I would have really appreciated a reminder email about that patch in particular (I honestly had forgotten about it by the time the review email came) but it would not have been helpful to mark it review-. I would have just marked it review? again, which given the eventual outcome (r+ with some comments) would have been the right move. We should make it clearer to new contributors that it is the role of the patch submitter to sell it to the project. This means making it clear why the patch is an improvement, making it easy for the reviewer to understand, and making a convincing argument that the patch is correct by thorough testing. This also tends to mean figuring out who could review a patch and chasing them down via email or IRC. I think if contributors thought in these terms then it would naturally encourage the behaviors reviewers like - breaking patches up, providing more test cases, and clearly explaining what a patch is trying to do. - James On Wed, Jul 21, 2010 at 10:17 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Jul 21, 2010 at 2:40 PM, Ojan Vafai o...@chromium.org wrote: There are currently 38 (of 171 total) patches in the review queue where the bugs have not been modified in over 1 month old. I propose we have a bot that educates people about writing easy to review patches and auto-rejects any patches in bugs that haven't been touched in over a month. I'd love to see a bot that educates people but I'm not sure if we should auto-rejects patches that are simply old. Maybe we need a little bit more of graduality like pinging the author of the patch first, and then make it obsolete if nobody responds etc... This is just an initial proposal. I'm not wed to any of the details of how this would work. I do think that auto-rejecting old patches is valuable to the project as a whole. Having the review queue be so large makes it daunting for any reviewer to try and tackle it. I'd agree that having a really large review queue isn't ideal. Maybe we can customize the review queue so that it only shows patches of your expertise. Best, Ryosuke Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Hi Yong, Sorry for r-ing a bunch of your patches last night. I tried to give you constructive comments. As you might be aware, we have a style linter now: ./WebKitTools/Scripts/check-webkit-style It might help us skip past some of the trivial review comments (like improper indenting of initializer lists). For the patches with licensing issues, I'm not sure how best to proceed. Adam On Wed, Sep 2, 2009 at 8:29 AM, Yong Liyong...@torchmobile.com wrote: Thanks a lot for spending time on reviewing. As a developer, I hope I can have more constructive comments for r-. I understand it's a hard work and truly appreciate all of you who spend time on it. -Yong - Original Message - From: Eric Seidel To: WebKit Development Sent: Wednesday, September 02, 2009 6:10 AM Subject: [webkit-dev] Review Queue Just your friendly reminder that the review-queue is out of control again. We're @ 54 (was 84 on Monday): https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F If a reviewers could each take a stab at r-'ing (or r+'ing!) a few patches today we'd be back under 30 in no time... -eric p.s. Thanks to Adam Barth (and others) for applying the flaming sword of r- last night. It's slightly more under control. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Honestly, I was probably a bit aggressive in r-ing your patches. Hopefully you won't take that personally. My hope was to get the ball rolling again because the work on the WINCE port seemed to have stalled out. Having a bunch of r? patches languishing in the review queue isn't really helpful to anyone. +1! ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Review Queue needs some more attention
We have close to 100 patches that need attention in the review queue. http://www.webkit.org/pending-review I count five or six for the Qt port. Eight for the GTK port. Several for the Haiku port where a decision should probably be made. And several for the ARM Jit work and the custom memory allocator. I'm going to try and go through what I can, but if any other reviewers have some time... Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review queue needs love
I reviewed quite a few last night as well. At the moment there appear to be a large number of chromium, gtk, and qt specific patches up for review -- it would be great if reviewers for those ports went through them all :D --Oliver On Jun 18, 2009, at 2:27 PM, Adam Barth wrote: I'll do my best tonight. Adam On Thu, Jun 18, 2009 at 9:26 AM, Maciej Stachowiakm...@apple.com wrote: 54 bugs with patches to review: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F I reviewed a few just now, I'll try to do more today. Anyone else? Cheers, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review queue needs love
On Thursday 18 June 2009 05:39:04 pm Oliver Hunt wrote: I reviewed quite a few last night as well. At the moment there appear to be a large number of chromium, gtk, and qt specific patches up for review -- it would be great if reviewers for those ports went through them all :D I went through all the Qt ones today after Maciej made his request. I think there is only one outstanding Qt related one at the moment... Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Ok, I agree that a sectioned queue would help. There is no need to see the Gtk patches in the list: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F (which is back up to 40, btw!) But we should be able to keep core (nearly) empty if it were kept separate. -eric On Fri, May 22, 2009 at 11:22 PM, Eric Seidele...@webkit.org wrote: We're down to 30 bugs: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F With 36 patches total: curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 15 of those are Gtk-only bugs. Need some help from the Gtk reviewers on this one. Thanks again to those who have pitched in so far. -eric On Sat, May 23, 2009 at 12:20 AM, Eric Seidel e...@webkit.org wrote: Reviewed more before bed. We're down to: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F 50 and curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 61 respectively. Still awful, but much much better than yesterday. -eric On Fri, May 22, 2009 at 7:40 PM, Maciej Stachowiak m...@apple.com wrote: On May 22, 2009, at 2:19 AM, Eric Seidel wrote: Update: We're down to 74 patches now. Thanks especially to Maciej for all his reviewing this evening: curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l] 74 Still a long way to go. FWIW I prefer this query, which counts by bug and gives a count of 57: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F It looks like about half the remaining bugs are Qt or Gtk specific, and likely need someone who is expert on those ports to look at them. I don't feel comfortable reviewing things that affect Qt or Gtk APIs especially. - Maciej -eric On Fri, May 22, 2009 at 12:27 PM, Eric Seidel e...@webkit.org wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
We're down to 30 bugs: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F With 36 patches total: curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 15 of those are Gtk-only bugs. Need some help from the Gtk reviewers on this one. Thanks again to those who have pitched in so far. -eric On Sat, May 23, 2009 at 12:20 AM, Eric Seidel e...@webkit.org wrote: Reviewed more before bed. We're down to: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F 50 and curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 61 respectively. Still awful, but much much better than yesterday. -eric On Fri, May 22, 2009 at 7:40 PM, Maciej Stachowiak m...@apple.com wrote: On May 22, 2009, at 2:19 AM, Eric Seidel wrote: Update: We're down to 74 patches now. Thanks especially to Maciej for all his reviewing this evening: curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l] 74 Still a long way to go. FWIW I prefer this query, which counts by bug and gives a count of 57: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F It looks like about half the remaining bugs are Qt or Gtk specific, and likely need someone who is expert on those ports to look at them. I don't feel comfortable reviewing things that affect Qt or Gtk APIs especially. - Maciej -eric On Fri, May 22, 2009 at 12:27 PM, Eric Seidel e...@webkit.org wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Hi Eric, On May 21, 2009, at 9:47 PM, Eric Seidel wrote: Interesting analogy. However, closing means to me that the community is done with the bug. Denying a patch because no one's working on it anymore (aka, no one is there to respond to review comments even if you make them) is not the same as closing a bug. There is a forgotten patches link on the nightly start page iirc which shows all the r-'d patches. :) http://nightly.webkit.org/start/ I've certainly looked through that list for patches to finish before. Maybe I'm the only one. Many of the bugs we see languish in the review queue are just too big to be easily reviewed. I don't think we encourage enough decisive action from reviewers (like just r-ing a patch because it's too big to review). At least an r- action gives the contributer something more than silence. :) This could of course all be avoided if we got our review queue down to something manageable. :) My issue with this is that doing r- doesn't really solve the problem, it just makes the patch magically seem to disappear from the review queue. In general, I think it does make sense to prune really old patches and bugs periodically, but 2 weeks is way too fast IMHO (and even 1 month probably), even considering bit rot, and what I don't understand is why you want to put the burden on patch submitters to resubmit because their patches aren't getting reviewed? I don't see how this helps patches to get reviewed; in fact, quite the opposite - they may magically disappear from the queue on just the day someone might have decided to check the queue and actually review it. I do understand the frustration, as I've had my fair share of patches sit for a while awaiting review, but from what I can tell all your changes will do is just make it even more work for me to try and get a review. In addition to going around trying to find someone to review my patch, now I have to periodically resubmit too. And this gets my patch reviewed faster how? I do think there are real issues to be discussed here, and I think some of the ideas presented in other posts in this thread are worth taking a look at, but I just feel this approach is more a knee-jerk reaction rather than a real strategy to fix the review queue problem. Regards, Kevin We're down to 95 after today. (We were at 144 two days ago!) curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 95 -eric On Fri, May 22, 2009 at 2:38 PM, Maciej Stachowiak m...@apple.com wrote: On May 21, 2009, at 9:36 PM, Geoffrey Garen wrote: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to review their patches. Posting patches to the review queue does not automatically get you a review, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the review queue untouched I view that as the responsible reviewers' implicit rejection of the patch. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get an r- on my patches than have them sit ignored for multiple weeks at a time. Let's examine these statements in a broader light, substituting bug database for review queue and bug for patch: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to fix their bugs. Filing bugs to the bug database does not automatically get you a fix, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the bug database untouched, I view that as the responsible reviewers' implicit rejection of the bug. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get a closed on my bugs than have them sit ignored for multiple weeks at a time. So, Eric, should we close all bugs that are older than 2 weeks? I thought of the same analogy, and for this reason I disagree with Eric's proposed change. Marking patches r- without review feedback is impolite to the patch submitter, and loses valuable information. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 22, 2009, at 2:19 AM, Eric Seidel wrote: Update: We're down to 74 patches now. Thanks especially to Maciej for all his reviewing this evening: curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l] 74 Still a long way to go. FWIW I prefer this query, which counts by bug and gives a count of 57: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F It looks like about half the remaining bugs are Qt or Gtk specific, and likely need someone who is expert on those ports to look at them. I don't feel comfortable reviewing things that affect Qt or Gtk APIs especially. - Maciej -eric On Fri, May 22, 2009 at 12:27 PM, Eric Seidel e...@webkit.org wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Am 22.05.2009 um 06:41 schrieb Maciej Stachowiak: On May 21, 2009, at 9:18 PM, Ojan Vafai wrote: It makes no sense to me to r- a patch because reviewers don't have time to review it. It put incentive in the wrong place. There are other solutions to this problem that put incentive in the right place (i.e. with reviewers). I don't necessarily think these are good ideas, but I'll throw them out there. 1. If the review queue gets too large, close the tree to commits until the queue is at 80% of that too large number. 2. Automate landing commits (e.g. click a button off the commit queue) so that it's easier to land commits and keep the commit queue small. 3. Give some kind of very visible notice on the waterfall when the review queue gets too large (e.g. make the background color of the whole page red). 4. Auto-assign reviews that haven't gotten reviewed after two weeks to a reviewer and have that person be responsible for reviewing it in a timely manner (this gives personal responsibility over the queue length instead of the current distributed responsibility). I'm sure there are a ton of other possible solutions to this problem that don't punish people sending patches who have little control over this situation. I imagine doing 3 and 4 would make a significant difference. I discussed the review backlog with Mark Rowe earlier, and we came up with another idea that may help. This would be to categorize the review queues. Perhaps we could get bugzilla to show a separate review queue per component. So for example there would be queues for CSS, JavaScriptCore, WebKit Qt, etc. Then we could assign specific people to be responsible for that queue. That doesn't mean these are the only people who can review, but we would know who to badger if certain queues get backlogs. I think this is a very wise idea. If we'd combine this with a review queue supervisor per component it with certainly help to avoid backlogs. For instance the supervisor could get a mail all two weeks listing the patches waiting for review. Either the supervisor takes care of the review or delegates to other reviewers. The idea is that at least someone notices the queue is very long. The last times this came up on the mailing list I think it was either Eric or Maciej notifying the crowd about the state of the review queue. This definately needs to be changed. So, you get my voice for creating categorized review queues. The positive side-effect is that I don't need to scan long time for patches that interesst me (aka. that I'm qualified for review), but rather get the list of patches immediately in a single view. I think it's great that Eric brought up this topic again, as I think we can improve the situation much better with not that much effort. Have a nice day, Niko ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On Thu, 2009-05-21 at 21:41 -0700, Maciej Stachowiak wrote: I discussed the review backlog with Mark Rowe earlier, and we came up with another idea that may help. This would be to categorize the review queues. Perhaps we could get bugzilla to show a separate review queue per component. So for example there would be queues for CSS, JavaScriptCore, WebKit Qt, etc. Then we could assign specific people to be responsible for that queue. That doesn't mean these are the only people who can review, but we would know who to badger if certain queues get backlogs. This would rock. I have my own bugzilla search to try to find GTK+-only review requests. It's not very reliable, though. Having this correctly categorized in the review queue itself would be much better. See you, -- Gustavo Noronha g...@gnome.org GNOME contributor ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Reviewed more before bed. We're down to: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F 50 and curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 61 respectively. Still awful, but much much better than yesterday. -eric On Fri, May 22, 2009 at 7:40 PM, Maciej Stachowiak m...@apple.com wrote: On May 22, 2009, at 2:19 AM, Eric Seidel wrote: Update: We're down to 74 patches now. Thanks especially to Maciej for all his reviewing this evening: curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l] 74 Still a long way to go. FWIW I prefer this query, which counts by bug and gives a count of 57: https://bugs.webkit.org/buglist.cgi?field0-0-0=flagtypes.nametype0-0-0=equalsvalue0-0-0=review%3F It looks like about half the remaining bugs are Qt or Gtk specific, and likely need someone who is expert on those ports to look at them. I don't feel comfortable reviewing things that affect Qt or Gtk APIs especially. - Maciej -eric On Fri, May 22, 2009 at 12:27 PM, Eric Seidel e...@webkit.org wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Review Queue
Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 21, 2009, at 7:27 PM, Eric Seidel wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. While it's great to get more reviews done (and I encourage other reviewers to get cracking as well), I don't think it's appropriate to r- patches that you don't know how to review, simply because no one else has reviewed them yet. It is better to have an honest backlog than to sweep things under the carpet. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
I think it's better to get things out of the queue then to leave them rot. Your review are most welcome. :) -eric On Fri, May 22, 2009 at 12:48 PM, Maciej Stachowiak m...@apple.com wrote: On May 21, 2009, at 7:27 PM, Eric Seidel wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. While it's great to get more reviews done (and I encourage other reviewers to get cracking as well), I don't think it's appropriate to r- patches that you don't know how to review, simply because no one else has reviewed them yet. It is better to have an honest backlog than to sweep things under the carpet. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 21, 2009, at 7:57 PM, Eric Seidel wrote: I think it's better to get things out of the queue then to leave them rot. But it's not the patch submitter's fault if the reviewers have been delinquent in review. And making them resubmit the same patch after a blanket r- is useless busywork. Per our policy you shouldn't mark a patch r- unless you are either requesting revisions or rejecting the whole concept of the patch http://webkit.org/coding/ contributing.html. So if you r-'d any patches without giving review comments, please restore them. Regards, Maciej Your review are most welcome. :) -eric On Fri, May 22, 2009 at 12:48 PM, Maciej Stachowiak m...@apple.com wrote: On May 21, 2009, at 7:27 PM, Eric Seidel wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. While it's great to get more reviews done (and I encourage other reviewers to get cracking as well), I don't think it's appropriate to r- patches that you don't know how to review, simply because no one else has reviewed them yet. It is better to have an honest backlog than to sweep things under the carpet. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Sorry I missed on on IRC, I would have been happy to chat further. Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to review their patches. Posting patches to the review queue does not automatically get you a review, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the review queue untouched I view that as the responsible reviewers' implicit rejection of the patch. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get an r- on my patches than have them sit ignored for multiple weeks at a time. -eric http://webkit.org/coding/contributing.html: Patch review Once you have a patch file, it must be reviewed by one of the approved WebKit reviewers. To request a review, attach the patch to the bug report, and mark the patch with the flag review:?. The reviewer will typically either approve the patch (by responding with an r=me in the bug report or in e-mail and marking the patch review:+) or request revisions to the patch (and mark the patch review:-). In rare cases a patch may be permanently rejected, meaning that the reviewer believes the feature should never be committed to the tree. The review process can consist of multiple iterations between you and the reviewer as revisions are made to your patch. On Fri, May 22, 2009 at 1:22 PM, Maciej Stachowiak m...@apple.com wrote: On May 21, 2009, at 7:57 PM, Eric Seidel wrote: I think it's better to get things out of the queue then to leave them rot. But it's not the patch submitter's fault if the reviewers have been delinquent in review. And making them resubmit the same patch after a blanket r- is useless busywork. Per our policy you shouldn't mark a patch r- unless you are either requesting revisions or rejecting the whole concept of the patch http://webkit.org/coding/contributing.html. So if you r-'d any patches without giving review comments, please restore them. Regards, Maciej Your review are most welcome. :) -eric On Fri, May 22, 2009 at 12:48 PM, Maciej Stachowiak m...@apple.com wrote: On May 21, 2009, at 7:27 PM, Eric Seidel wrote: Our review process seems to be failing. As a reviewer, let me extend my apologies to the WebKit community as I am part of this failure. We have over 100 patches in the review queue at the moment: http://webkit.org/pending-review I've started going through the list and reviewing what patches I can. I'm also marking r- all patches I can't review which have had no comments in the last 2 weeks. While it's great to get more reviews done (and I encourage other reviewers to get cracking as well), I don't think it's appropriate to r- patches that you don't know how to review, simply because no one else has reviewed them yet. It is better to have an honest backlog than to sweep things under the carpet. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 21, 2009, at 9:36 PM, Geoffrey Garen wrote: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to review their patches. Posting patches to the review queue does not automatically get you a review, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the review queue untouched I view that as the responsible reviewers' implicit rejection of the patch. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get an r- on my patches than have them sit ignored for multiple weeks at a time. Let's examine these statements in a broader light, substituting bug database for review queue and bug for patch: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to fix their bugs. Filing bugs to the bug database does not automatically get you a fix, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the bug database untouched, I view that as the responsible reviewers' implicit rejection of the bug. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get a closed on my bugs than have them sit ignored for multiple weeks at a time. So, Eric, should we close all bugs that are older than 2 weeks? I thought of the same analogy, and for this reason I disagree with Eric's proposed change. Marking patches r- without review feedback is impolite to the patch submitter, and loses valuable information. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 21, 2009, at 9:18 PM, Ojan Vafai wrote: It makes no sense to me to r- a patch because reviewers don't have time to review it. It put incentive in the wrong place. There are other solutions to this problem that put incentive in the right place (i.e. with reviewers). I don't necessarily think these are good ideas, but I'll throw them out there. 1. If the review queue gets too large, close the tree to commits until the queue is at 80% of that too large number. 2. Automate landing commits (e.g. click a button off the commit queue) so that it's easier to land commits and keep the commit queue small. 3. Give some kind of very visible notice on the waterfall when the review queue gets too large (e.g. make the background color of the whole page red). 4. Auto-assign reviews that haven't gotten reviewed after two weeks to a reviewer and have that person be responsible for reviewing it in a timely manner (this gives personal responsibility over the queue length instead of the current distributed responsibility). I'm sure there are a ton of other possible solutions to this problem that don't punish people sending patches who have little control over this situation. I imagine doing 3 and 4 would make a significant difference. I discussed the review backlog with Mark Rowe earlier, and we came up with another idea that may help. This would be to categorize the review queues. Perhaps we could get bugzilla to show a separate review queue per component. So for example there would be queues for CSS, JavaScriptCore, WebKit Qt, etc. Then we could assign specific people to be responsible for that queue. That doesn't mean these are the only people who can review, but we would know who to badger if certain queues get backlogs. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 21, 2009, at 9:10 PM, Peter Kasting wrote: On Thu, May 21, 2009 at 8:52 PM, Eric Seidel e...@webkit.org wrote: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to review their patches. Posting patches to the review queue does not automatically get you a review, except occasionally by Darin Adler or myself. FWIW, I've always been told that WebKit etiquette is to not choose a reviewer (even when you know who would be good) and just mark a patch r?. (This is a stark contrast to the Mozilla way of _always_ selecting a reviewer.) It would probably be good to clarify this kind of thing. It's better not to explicitly select a reviewer, since almost always there will be several people qualified. However, it is totally OK to ask a specific person if they would consider reviewing out of band of the bug system, or to Cc possible qualified reviewers on the bug. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
Interesting analogy. However, closing means to me that the community is done with the bug. Denying a patch because no one's working on it anymore (aka, no one is there to respond to review comments even if you make them) is not the same as closing a bug. There is a forgotten patches link on the nightly start page iirc which shows all the r-'d patches. :) http://nightly.webkit.org/start/ I've certainly looked through that list for patches to finish before. Maybe I'm the only one. Many of the bugs we see languish in the review queue are just too big to be easily reviewed. I don't think we encourage enough decisive action from reviewers (like just r-ing a patch because it's too big to review). At least an r- action gives the contributer something more than silence. :) This could of course all be avoided if we got our review queue down to something manageable. :) We're down to 95 after today. (We were at 144 two days ago!) curl -s https://bugs.webkit.org/request.cgi; | grep PDT | wc -l 95 -eric On Fri, May 22, 2009 at 2:38 PM, Maciej Stachowiak m...@apple.com wrote: On May 21, 2009, at 9:36 PM, Geoffrey Garen wrote: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to review their patches. Posting patches to the review queue does not automatically get you a review, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the review queue untouched I view that as the responsible reviewers' implicit rejection of the patch. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get an r- on my patches than have them sit ignored for multiple weeks at a time. Let's examine these statements in a broader light, substituting bug database for review queue and bug for patch: Our current defacto policy requires involvement on both sides. Submitters need to be involved in finding people to fix their bugs. Filing bugs to the bug database does not automatically get you a fix, except occasionally by Darin Adler or myself. If a bug totally stalls, and is sitting in the bug database untouched, I view that as the responsible reviewers' implicit rejection of the bug. I, as a responsible reviewer, am simply making explicit that implicit rejection. Personally, I'd rather get a closed on my bugs than have them sit ignored for multiple weeks at a time. So, Eric, should we close all bugs that are older than 2 weeks? I thought of the same analogy, and for this reason I disagree with Eric's proposed change. Marking patches r- without review feedback is impolite to the patch submitter, and loses valuable information. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On May 21, 2009, at 9:47 PM, Eric Seidel wrote: Interesting analogy. However, closing means to me that the community is done with the bug. Denying a patch because no one's working on it anymore (aka, no one is there to respond to review comments even if you make them) is not the same as closing a bug. There is a forgotten patches link on the nightly start page iirc which shows all the r-'d patches. :) http://nightly.webkit.org/start/ I've certainly looked through that list for patches to finish before. Maybe I'm the only one. Many of the bugs we see languish in the review queue are just too big to be easily reviewed. I don't think we encourage enough decisive action from reviewers (like just r-ing a patch because it's too big to review). At least an r- action gives the contributer something more than silence. :) If you want to r- a patch for a reason, such as being too big, or having feedback already that hasn't been addressed, that's fine. But I think it would be a bad idea to reject patches just because they haven't been reviewed for too long. And the other folks who have spoken up so far seem to agree. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Review Queue
On Thu, May 21, 2009 at 10:08 PM, Maciej Stachowiak m...@apple.com wrote: If you want to r- a patch for a reason, such as being too big, or having feedback already that hasn't been addressed, that's fine. But I think it would be a bad idea to reject patches just because they haven't been reviewed for too long. And the other folks who have spoken up so far seem to agree. I'm on the fence overall, but I can understand eseidel's position, so I'll chime in so he's not all alone. To use that same bug analogy, Mozilla actually _did_ that with their bug database and found it extremely valuable. Closing old bugs meant that the 1% that have serious, interested parties behind them that could still reproduce spoke up, and those bugs stayed alive -- noticeably increasing the quality of the bug pool. While I don't think the same effect applies to reviews two weeks old, it certainly does after, say, two months -- and in the Chromium database I routinely ping people on old reviews that have languished and eventually close them. If the person submitting the patch is interested, and the message with the r- is clear, getting an r- that says if this still applies please re-ping is at the very worst an annoyance, not a disaster. If the timeout was increased from two weeks to one month, and coupled with a push that got the queue to zero patches and then implemented policies to help it stay low in the future (which both Chromium and Mozilla lack, so you don't feel too bad), I would be fully supportive. As-is, I'm not sure it helps as much, but I'm not ticked. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
