43948 2010-08-12 22:23:35 -0700 Bugzilla "review" action should not paste full diff as comment by default 2010-08-19 11:24:08 -0700 1 1 1 Unclassified WebKit WebKit Website 528+ (Nightly build) PC OS X 10.5 NEW P2 Normal --- 1 eric webkit-unassigned abarth ap aroben darin jparent mark oldest_to_newest 264173 0 eric 2010-08-12 22:23:35 -0700 Bugzilla "review" action should not paste full diff as comment by default This makes it very easy for users to kill bugs: https://bugs.webkit.org/show_bug.cgi?id=43595#c24 We have working line-by-line diffs, we should make that the default behavior, and replace the "clear main comment box" button with a "paste full diff into comment box" button for anyone who wants this old behavior. 264237 1 abarth 2010-08-13 01:33:27 -0700 Darin might have an opinion here. Last time I touched this I broke his workflow, and I'd rather not do that again. :) 264348 2 ap 2010-08-13 07:05:14 -0700 I prefer to not use the line by line diffs, and edit the full diff instead. Not that I find my current workflow efficient, but removing the full diff would make it even worse. 264363 3 eric 2010-08-13 08:10:52 -0700 There needs to be some sort of safety net though. It's *way* too easy to spam a bug with the "edit the diff" behavior. Making it non-default is one way to give it a safety net. But not the only way. Mark's mistake is not the only bug which has died due to someone making a huge comment from our "review patch" tool. Even people who understand the tool don't edit down the diff nearly enough and make gigantic comments. 264414 4 ojan 2010-08-13 09:55:33 -0700 I imagine we could just detect this case and deal with it. For example, if the number of diff lines before the first review comment or after the last one are >20, we could truncate them to 20. This would be a simple tweak to the JS on the review page. 264425 5 abarth 2010-08-13 10:13:35 -0700 Maybe we should clear the big comment box the first time someone saves a line-by-line comment if the big box still has the original contents. 265591 6 eric 2010-08-17 09:10:27 -0700 https://bugs.webkit.org/show_bug.cgi?id=43055 was also just destroyed by this poor default. We really need to remove this noose from our review system IMO. 265688 7 darin 2010-08-17 12:11:06 -0700 I edit the full diff too. Before we added the button to add individual diffs, this error was rare, and the script I wrote to remove the full diff if you don’t edit it, made it almost never happen. The change here was adding that new workflow. Currently, that gets in my way when I want to search for things within the patch. I click to select a word and copy it to search, and a text field gets inserted into the patch. This is inconvenient. I don’t think the new feature for adding comments with individual lines to the patch review has a good UI yet. It confuses people and results in patches with full diffs. Please don’t fix this by removing the old workflow! 266716 8 eric 2010-08-19 07:19:21 -0700 Another victim: https://bugs.webkit.org/show_bug.cgi?id=44174#c6 266851 9 darin 2010-08-19 11:24:08 -0700 Someone should take my JavaScript to remove the diff if it's still there and make it handle more cases. Also, I would find it acceptable if pasting in the diff was triggered by a button instead of default. Generally speaking I think we are making that review page harder to understand and use with each change right now.