[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] WebKitTools/Script/generate-coverage-data
On 08/26/2010 11:56 PM, Holger Freyther wrote: Hi, the only change is to exclude the coverage option for ANGLE as libtool does not like these parameters. I have tested the scripts and they work, I will need to test the whole generate-coverage script tomorrow and will open a bug report... Darin was nice enough to review and I have landed it, please try it and tell me if you have any problems with the script. ___ 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
Re: [webkit-dev] We need OwnPtrHashMap
On Aug 28, 2010, at 10:57 PM, Darin Adler wrote: We need VectorOwnPtr too. It has similar issues to HashMap with OwnPtr values, including the ones mentioned by Maciej. For one example, look at CSSParser::m_floatingMediaQueryExpList. VectorOwnPtr actually works[1], and I have an almost complete patch to fully OwnPtr-ize MediaQueryExp. The one problem is sorting - MediaQuery wants to sort the VectorMediaQuery* it gets with std::sort, followed by eliminating duplicates. A sort that solely uses swaps would work, but std::sort wants to make copies of the elements at times. I also tried to think of ways to cheat by copying to a vector of raw pointers and back, and it could be made to work with sufficiently aggressive use of leakPtr and adoptPtr, but at the cost of two extra copies. Yet another possibility is to use a hash to do the de-duping instead of sorting; I can't tell from context if the sorting is needed for any purpose other than subsequent de-duping. If you can help me think of a good solution for this I'll post my patch. Regards, Maciej [1] It works because: (a) VectorTraits already takes care of all internal copies that are actually moves, by cheating and doing them with memcpy. (b) Reads, and writes of an existing slot, all operate via a reference to the element type, so you can use - or .get() on a returned element just fine, and you can assign in a PassOwnPtrT to an OwnPtrT. (c) append() and similar methods that add an element to a not-yet-existing slot all are templatized on the type of the element being added, so appending a PassOwnPtr works. The Hash templates are more complicated, so they probably won't just work automatically. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] We need OwnPtrHashMap
On Aug 29, 2010, at 9:14 PM, Maciej Stachowiak wrote: Yet another possibility is to use a hash to do the de-duping instead of sorting; I can't tell from context if the sorting is needed for any purpose other than subsequent de-duping. Turns out this doesn't work - the CSS Media Queries spec specifically requires serializing in sorted order and we have a test to that effect: http://dev.w3.org/csswg/cssom/#serializing-media-queries https://bugs.webkit.org/show_bug.cgi?id=39220 So it's down to violating the type system or writing my own sort. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
