On Friday, June 5, 2009 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.
But I'm a reviewer. Don't you want to be selling me on the virtues of
Reitveld? :)
What happens if I choose to update a patch in Bugzilla instead of reitveld
(assuming I'm not required to use Reitveld as you say)? Will Bugzilla push the
status of the patch back to Reitveld?
> 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.
The one major thing it would buy us is less maintenance--adding another web
site would double the amount of maintenance for the bug system. I can easily
imagine that upgrading one would break integration with the other and
vice-versa. Is there something I'm missing that would mitigate the risk of
additional maintenance and break-on-upgrade issues?
It's obvious that a lot of work has gone into Reitveld, and I'm sure it's a
great tool. I just think it's a shame that no one has stepped up to provide
similar functionality for Bugzilla, thereby improving the status quo for all
users of this popular open source tool.
Dave
________________________________
From: Ojan Vafai <[email protected]>
To: Darren VanBuren <[email protected]>
Cc: David Kilzer <[email protected]>; Jeremy Orlow <[email protected]>;
WebKit Development <[email protected]>
Sent: Friday, June 5, 2009 7:25:40 PM
Subject: Re: [webkit-dev] to reitveld or not to reitveld
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 <[email protected]> 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
[email protected]
====================
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 <[email protected]>
To: Ojan Vafai <[email protected]>
Cc: WebKit Development <[email protected]>
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 <[email protected]> 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/%C2%A0is 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
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
