[webkit-dev] to reitveld or not to reitveld

Fri Jun 5 19:02:21 PDT 2009

Surprisingly, the bug isn't a duplicate, or if there is a dupe, it  
isn't filed correctly.

But I agree that any code review tool should be integrated with  
bugs.webkit.org, otherwise there would be a huge disorganized mess and  
it wouldn't be any better.

Bugzilla wouldn't be hard to extend for this purpose, just adding a  
field for review status and then making whatever code review tool you  
chose update Bugzilla solves (b).

Some modifications in the tool could also make it attach the patches  
to a bug, and you could also update any field in the bug.

I mean, retiveld seems like a wonderful tool, it seems like something  
that would extend Bugzilla quite nicely. Pushing data to Bugzilla can  
simply be done with XML-RPC according to this page on bugzilla.org: http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Server/XMLRPC.html 
  and there's plenty of XML libraries for Python you could use to work  
over XML-RPC.

Darren VanBuren
onekopaka at gmail.com
====================
http://oks.tumblr.com/

On Jun 5, 2009, at 6:21 PM, David Kilzer wrote:

> 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 <jorlow at chromium.org>
> To: Ojan Vafai <ojan at chromium.org>
> Cc: WebKit Development <webkit-dev at 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 <ojan at 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 at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20090605/0762f861/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: This is a digitally signed message part
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20090605/0762f861/attachment.bin>