[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 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 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. I'm not sure who objects to new features being added to Review Patch, but I don't like this change: 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. 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". 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 4:16 PM, Maciej Stachowiak 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). > 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. 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: > On Sun, Aug 29, 2010 at 4:16 PM, Maciej Stachowiak 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 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 Vector too. It has similar issues to HashMap with OwnPtr > values, including the ones mentioned by Maciej. > > For one example, look at CSSParser::m_floatingMediaQueryExpList. Vector 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 Vector 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 PassOwnPtr to an OwnPtr&. (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
Re: [webkit-dev] Changes to line-by-line code reviews
On Sun, Aug 29, 2010 at 8:08 PM, Darin Adler wrote: > 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. Yeah, my goal in working on the tool is to make it awesome enough that you're happy with it. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] deduplicate-results
On Sat, Aug 28, 2010 at 10:47 AM, Eric Seidel wrote: > I'm very glad to see work being done to remove some of our redundant > layout test results: > http://trac.webkit.org/changeset/66124 > > I'm curious what the status of that work is. Do we expect all > duplicates are gone now? or just chromium's? The script as written was Chromium-agnostic (it uses the layout test fallback lists from the normal WebKit scripts). I think Tony just wasn't completely confident it was going to do the right thing so he was deleting only a subdirectory at a time. (Last week he found and fixed a bug in it: when you find duplicate results for platform A that falls back on platform C, you need to consider intermediate platform B's results.) If you have a WebKit build and some time to spare, you can try deleting the redundant tests for your platform that it suggests deleting and seeing if your platform still passes the tests to verify before checking in the deletes. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
