I think this is a great direction to move in, but (IMO) any such
tool should be tightly integrated with bugs.webkit.org so that (a)
you don't have to post the same patch more than once, (b) the
review status of the patch is visible in bugs.webkit.org without
clicking on a link and (c) it's easy to switch between reviewing
the patch and updating the bug itself.
I just filed a Bugzilla bug about adding such a feature to Bugzilla
itself (although I wouldn't be surprised if it's a dupe):
Bugzilla needs better patch review process with annotations and
versioned patches
<https://bugzilla.mozilla.org/show_bug.cgi?id=496670>
Dave
From: Jeremy Orlow <jor...@chromium.org>
To: Ojan Vafai <o...@chromium.org>
Cc: WebKit Development <webkit-dev@lists.webkit.org>
Sent: Friday, June 5, 2009 5:08:47 PM
Subject: Re: [webkit-dev] to reitveld or not to reitveld
For what it's worth, I definitely think a tool like reitveld would
help the code review process. Inline comments and more context
than the couple lines the diff provides are really, really helpful.
On Fri, Jun 5, 2009 at 9:25 AM, Ojan Vafai <o...@chromium.org> wrote:
Sorry in advance for the long email. I'm trying to be thorough.
There's been a lot of discussion on #webkit about possibly using a
code review tool like reitveld for webkit reviews. There's been
various suggestions and a few misunderstandings, so it seems worth
having a more formal discussion about this with the larger WebKit
community.
The things I'd like to assess here are:
1. Pros/Cons of using a system like reitveld. I listed some below.
Please add any that I missed.
2. Whether the WebKit community is interested in pursuing something
like this.
3. If there is interest, what is the best way to move forward.
WHAT IS REITVELD
It's a code review tool. Reitveld doesn't allow you to do anything
that is impossible with the current review process, however, it
makes the review process more efficient and less error-prone. As
such, it makes it easier and less time-consuming to do good,
thorough code reviews.
The basic gist of reitveld is that it allows you to put comments
inline and send them all in one chunk. Then it lets the reviewee
easily respond to each comment individually and send all the
responses in one chunk.
EXAMPLE CHROMIUM PATCH
http://codereview.chromium.org/119103
Note that you can view the patch in each version that was uploaded
and that you can diff between versions. Also, if a comment was made
in the version you were looking at, then you can see all the
comments/responses.
To see this nicely, under "Delta from patch set" in patch set 3,
click on 2. That is where most of the comments in this review were
made. For example, http://codereview.chromium.org/119103/diff2/14:27/29
. You can see all the comments and responses along with the diff in
the patch to see that the reviewer comments were implemented as
intended.
Keyboard shortcuts to try out:
-n/p to switch between diff chunks
-shift n/p to switch between comments
-j/k to go to the next/previous file
*Please don't actually click the "Publish all my drafts" button on
the publish page as you'll be modifying a real code review.*
Other things to try
-try the side-by-side diff and the unified diff views
-adding comments (double click)
-replying to comments
-go to the publish page (click the publish link or type "m")
Also note that the "Committed" URL is automatically added when the
patch is committed and the reitveld issue is marked closed. So
there isn't extra overhead in maintaining list of outstanding code
reviews.
HOW TO TRY IT OUT
Here's the process for trying out reitveld with a webkit patch. The
current workflow is a bit janky, but some scripting and some simple
reitveld fixes would make this a lot more natural and automated
(e.g. chromium uses commandline "gcl upload" to put up a new patch).
1. Find a non-git patch
2. Go to http://codereview.chromium.org/new
3. Login with a Google account (e.g. any gmail or Google search
account)
4. On that page, type in a subject and paste in the URL to the
patch in the URL field.
5. Click "Create Issue"
There's a couple apparent bugs that are easily fixable:
1. The ChangeLog files don't get downloaded correct, so the diffs
don't work. This is an AppEngine problem that Chromium works around
with the gcl upload script.
2. With an old patch there are often diff chunk mistmatches, which
breaks the side-by-side diff view (you can use the unified diff in
those cases).
PROS
For the reviewer:
-easier to write thorough review comments since adding comments is
so light-weight
-easier to make sure that all review comments actually got
implemented
For the reviewee:
-easier to see which line the reviewer's comment addresses
-easier to make sure you've completed all the reviewer's comments
(you can mark them as "done" in reitveld as you go)
For everyone:
-efficient keyboard navigation (e.g. j/k to navigation between diff
chunks and n/p to navigate between files
-easier to follow the progression of a code review and see what
changed over the course of the review
-shows image files, so you can see before/after before commit
CONS (most of these are easy to fix/improve)
-There's no pretty printed view of all the files in the patch at
once that lets you insert comments
-The UI is a bit cluttered
-It takes using it for a couple patches before you're totally
comfortable with it
-Currently doesn't work with diffs generated by git
-Reitveld's current implementation requires running on AppEngine
-A couple issues with reitveld on appengine that Chromium uses a
script to workaround (line-endings differences and large files like
ChangeLogs don't upload correclty).
PATH FORWARD
As far as reitveld versus another code review tool, I don't have
strong opinions. I hear http://www.review-board.org/ is good, but I
haven't used it. One advantage of using reitveld is that a lot of
the work on reitveld was done by Chromium team members and so
modifying it to meet WebKit needs (e.g. running an instance that
isn't tied to Chromium, changing the UI, etc.) should be relatively
painless.
I think the transition to using a new tool would need to be
gradual, so that people can continue use the current webkit process
and not be forced into using a new tool. Like using git vs. svn, it
ought to be possible to use either process without putting an extra
burden on the webkit community.
If we agree that something like reitveld would be good for the
webkit community and wanted to make reitveld possible to use with
the current WebKit workflow, we *could* work out some of the simple
kinks listed above and do a lightweight integration with bugzilla
such that any updates to reitveld are reflected in bugzilla (but
not vice versa). There was pushback to this on #webkit, but it
seems to me like the simplest non-invasive way to let the webkit
community try this out to see if it's worth investing more time into.
That said, I'm totally open towards other approaches that aren't an
unreasonable amount of effort.
Ojan
P.S. For those interested, here's a talk about Mondrian [Reitveld's
closed-source predecessor] and how Google moved from text-diff base
reviews to Mondrian.
_______________________________________________
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