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
