Bug 43948
| Summary: | Bugzilla "review" action should not paste full diff as comment by default | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> |
| Component: | WebKit Website | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | abarth, ap, aroben, darin, jparent, mark |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | OS X 10.5 | ||
Eric Seidel (no email)
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.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Adam Barth
Darin might have an opinion here. Last time I touched this I broke his workflow, and I'd rather not do that again. :)
Alexey Proskuryakov
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.
Eric Seidel (no email)
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.
Ojan Vafai
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.
Adam Barth
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.
Eric Seidel (no email)
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.
Darin Adler
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!
Eric Seidel (no email)
Another victim:
https://bugs.webkit.org/show_bug.cgi?id=44174#c6
Darin Adler
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.