[webkit-dev] to reitveld or not to reitveld

Fri Jun 5 19:47:55 PDT 2009

Apparently, according to http://code.google.com/p/rietveld/, we've  
been spelling rietveld wrong.
Darren VanBuren
onekopaka at gmail.com
====================
http://oks.tumblr.com/

On Jun 5, 2009, at 7:25 PM, Ojan Vafai wrote:

> This is what I meant by "light-weight" integration. All the review  
> information would be reflected in the bugzilla bug. You would never  
> be required to use reitveld for anything.
>
> We would be able to:
> 1. Add a link to bugzilla that would take you to the reitveld code  
> review and upload the patch to reitveld if it hasn't been uploaded  
> already.
> 2. Have all comments published in reitveld be posted to the bug.
> 3. Have checkboxes in reitveld for r+, r- that would update bugzilla.
> 4. I think we can even have comments made directly to bugzilla be  
> reflected in reitveld by having a bot that monitors bugzilla update  
> emails.
>
> A review tool like reitveld is quite a bit of work. Adding similar  
> functionality to bugzilla itself is a non-trivial amount of work. I  
> don't see what integrating this functionality any more tightly into  
> bugzilla buys us that is worth the order(s) of magnitude more effort  
> that approach would take.
>
> Ojan
>
> On Sat, Jun 6, 2009 at 11:02 AM, Darren VanBuren  
> <onekopaka at gmail.com> wrote:
> 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/1af3df65/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/1af3df65/attachment.bin>