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

[Adam Barth]

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).

> 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