[webkit-dev] Changes to line-by-line code reviews
Based on some feedback, I'm going to try to improve the line-by-line review tool. I've landed the first iteration of the new design, which should be usable and have roughly the same functionality as the old design. I'll be adding new features shortly. The main difference is you now access the line-by-line review feature using the Formatted Diff link in bugs.webkit.org instead of the Review Patch link. I made this change so that folks who like the old Review Patch tool won't be bothered by the new tool. If you have feature requests, let me know. I'll post an update once the tool is awesomified. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Changes to line-by-line code reviews
It would be nice if you could select a block of code and have the comment be for that block -- last i looked the line by line review basically loses context for reviews as it pushes the comments to the bottom and only includes a single line of context. --Oliver On Aug 29, 2010, at 11:13 AM, Adam Barth wrote: Based on some feedback, I'm going to try to improve the line-by-line review tool. I've landed the first iteration of the new design, which should be usable and have roughly the same functionality as the old design. I'll be adding new features shortly. The main difference is you now access the line-by-line review feature using the Formatted Diff link in bugs.webkit.org instead of the Review Patch link. I made this change so that folks who like the old Review Patch tool won't be bothered by the new tool. If you have feature requests, let me know. I'll post an update once the tool is awesomified. Thanks, Adam ___ 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] Changes to line-by-line code reviews
So, I have two thoughts on that: 1) We can certainly do that. The trick will be making it discoverable. 2) I'd like the tool to read back in the state from the bug comments and re-populate the comments inline in the diff. That way you'll keep the context and can have threaded conversations in the diff. Adam On Sun, Aug 29, 2010 at 12:01 PM, Oliver Hunt oli...@apple.com wrote: It would be nice if you could select a block of code and have the comment be for that block -- last i looked the line by line review basically loses context for reviews as it pushes the comments to the bottom and only includes a single line of context. --Oliver On Aug 29, 2010, at 11:13 AM, Adam Barth wrote: Based on some feedback, I'm going to try to improve the line-by-line review tool. I've landed the first iteration of the new design, which should be usable and have roughly the same functionality as the old design. I'll be adding new features shortly. The main difference is you now access the line-by-line review feature using the Formatted Diff link in bugs.webkit.org instead of the Review Patch link. I made this change so that folks who like the old Review Patch tool won't be bothered by the new tool. If you have feature requests, let me know. I'll post an update once the tool is awesomified. Thanks, Adam ___ 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] Changes to line-by-line code reviews
On Aug 29, 2010, at 4:39 PM, Adam Barth wrote: On Sun, Aug 29, 2010 at 4:16 PM, Maciej Stachowiak m...@apple.com wrote: I'm not sure who objects to new features being added to Review Patch, but I don't like this change: There's a tention between folks who like line-by-line comments and (mostly) Darin, who likes the old Review Patch tool. Darin likes the giant textbox with the whole diff. I don't really understand, but I certainly have no wish to impede his use of the tools. 1) I'm used to having the click to add a comment feature in Review Patch, and would miss it if it was gone. 2) I think overloading Formatted Diff is wrong - it should remain a read-only view. I think the main remaining problems with the Review Patch page are the inability to give comments with multiple lines of context, and the excessive amount of space dedicated to things that are not the patch. The other problem is that reviews get hard to follow really fast when folks reply to review comments and the existing line-by-line editing requires about twice as many clicks as it needs. I don't have mockups to show, but what I had in mind was the following: 1) Use the whole page for the diff (basically remove the bottom half of the Review Patch screen). 2) When you're done writing line-by-line comments, show a lightbox that has the content that used to be at the bottom of the screen. That gives you a chance to enter high-level comments, read over your comments, and adjust the various flags. 3) The tools will then store the review in a comment, as before, which generates an email to the author of the patch. 4) The patch author can either read your review as a bugzilla comment, or they can look at the patch and the tool will show your comments inline in the diff where you wrote them. The author can respond by adding more comments inline, which again are stored as bugzilla comments, etc. In this approach, you can also imagine integration with the style checker and the EWS bots. The tools can show the style errors inline in the diff, as well as the compiler failures. The view is more that the diff is a living document with a bunch of layers (some of which are editable). That sounds like a good design to me. But it doesn't sound like a replacement for a read-only view of just the diff. If you add the ability to show and hide the lightbox at any time, it might even be a reasonable replacement even for people who like to type their review comments by hand. If changing the Review Patch page as needed would be too disruptive for some reason, I suggest using a new page instead of overloading Formatted Diff. We can certain add these features under a new name, if you think that would be the least disruptive. Sam already emailed me privately explaining how I broke one of his uses of the Formatted Diff, but I think I can fix that fairly easily by learning slightly more jQuery. I'm happy to build this off in a silo and then convince everyone how awesome it is once it actually is more awesome than the current tools. I suggest you start by making it a new page, and then we can decide whether to remove any of the existing pages in favor of the new one. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Changes to line-by-line code reviews
On Sun, Aug 29, 2010 at 5:07 PM, Maciej Stachowiak m...@apple.com wrote: On Aug 29, 2010, at 4:39 PM, Adam Barth wrote: I'm happy to build this off in a silo and then convince everyone how awesome it is once it actually is more awesome than the current tools. I suggest you start by making it a new page, and then we can decide whether to remove any of the existing pages in favor of the new one. Okiedokes. I might need help from some bugzilla experts to make a new page, but I'll give it at try. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Changes to line-by-line code reviews
On Aug 29, 2010, at 4:39 PM, Adam Barth wrote: Darin likes the giant textbox with the whole diff. Just to give context here: Some day the new tool might be good enough that I’ll change my mind. I use the new tool for about 1/10 of the patches I review and I plan to switch full time once the experience is good enough. But in the mean time I don’t want the prototype of the new tool to make the old one harder to use. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
