[webkit-dev] Changes to line-by-line code reviews

[Maciej Stachowiak]

On Aug 29, 2010, at 4:39 PM, Adam Barth wrote:

> On Sun, Aug 29, 2010 at 4:16 PM, Maciej Stachowiak <mjs at 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