Re: [webkit-dev] to reitveld or not to reitveld
On Sat, Jun 6, 2009 at 7:14 PM, Ojan Vafai o...@google.com wrote: On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com wrote: There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) It seems to me that all the issues raised with using reitveld are solvable. Assuming you all are OK with doing this iteratively instead of needing all the issues to be resolved on day one, I think we can probably start taking concrete steps forward. Maintenance/hosting is the biggest unanswered question. Would hosting this on reitveld (which does it's own replication and backups) be a deal-breaker? If so, there are a couple options a quick search brought up. One is http://arachnid.github.com/bdbdatastore/, which is linked to from the AppEngine blog as a way of hosting appengine apps on your own hardware. Another is http://code.google.com/appengine/articles/gae_backup_and_restore.html, which is a backup option for appengine. I'll reiterate that we've had great uptime and reliability for the Chromium reitveld instance on appengine. So, to me, hosting it yourself seems like extra work without much real-world benefit. Ojan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On 2009-06-11, at 15:16, Ojan Vafai wrote: On Sat, Jun 6, 2009 at 7:14 PM, Ojan Vafai o...@google.com wrote: On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com wrote: There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) It seems to me that all the issues raised with using reitveld are solvable. Assuming you all are OK with doing this iteratively instead of needing all the issues to be resolved on day one, I think we can probably start taking concrete steps forward. Given what has been said so far I'm still not clear why Rietvald is a better option than Review Board. - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On Thu, Jun 11, 2009 at 3:21 PM, Mark Rowe mr...@apple.com wrote: Given what has been said so far I'm still not clear why Rietvald is a better option than Review Board. I think it's because we have a bunch of people who are (a) familiar with using it and/or (b) work on it and can fix any problems, whereas neither is true for Review Board. Also I haven't really heard much significantly Review Board Rietveld, more the sense of we could try these both out. It seems like reading between the lines you're really uncomfortable with Rietveld in principle. Is there something about it that you feel is inherently not going to work well? PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
Mark Rowe wrote: On 2009-06-11, at 15:16, Ojan Vafai wrote: On Sat, Jun 6, 2009 at 7:14 PM, Ojan Vafai o...@google.com mailto:o...@google.com wrote: On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com mailto:mr...@apple.com wrote: There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) It seems to me that all the issues raised with using reitveld are solvable. Assuming you all are OK with doing this iteratively instead of needing all the issues to be resolved on day one, I think we can probably start taking concrete steps forward. Given what has been said so far I'm still not clear why Rietvald is a better option than Review Board. Well, I haven't heard anything concrete on why Review Board is better than rveld, either. All I've seen are some posts saying, You know, Review Board exists too. Is the UI better? Is the architecture better? Is the design very different? Is it easier to integrate with git? Exactly how much work is involved in hosting each of these solutions? The only thing concrete I've seen is that Review Board is self-hosting, while rveld is tied to AppEngine. Joe ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On Thu, Jun 11, 2009 at 4:50 PM, Joe Mason joe.ma...@torchmobile.comwrote: Mark Rowe wrote: On 2009-06-11, at 15:16, Ojan Vafai wrote: On Sat, Jun 6, 2009 at 7:14 PM, Ojan Vafai o...@google.com mailto: o...@google.com wrote: On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com mailto:mr...@apple.com wrote: There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) It seems to me that all the issues raised with using reitveld are solvable. Assuming you all are OK with doing this iteratively instead of needing all the issues to be resolved on day one, I think we can probably start taking concrete steps forward. Given what has been said so far I'm still not clear why Rietvald is a better option than Review Board. Well, I haven't heard anything concrete on why Review Board is better than rveld, either. All I've seen are some posts saying, You know, Review Board exists too. Is the UI better? Is the architecture better? Is the design very different? Is it easier to integrate with git? Exactly how much work is involved in hosting each of these solutions? The only thing concrete I've seen is that Review Board is self-hosting, while rveld is tied to AppEngine. ...and there's some that you could run rietveld without AppEngine infastructure if necessary. FWIW, another project I (used to) work on tried Review Board out about a year ago. We found it to be pretty clunky, so we continued just doing code reviews via diffs in email. Maybe it's improved since then. Either way, I believe the team is now using Rietveld for large reviews. J ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On Thu, Jun 11, 2009 at 5:19 PM, Jeremy Orlow jor...@chromium.org wrote: On Thu, Jun 11, 2009 at 4:50 PM, Joe Mason joe.ma...@torchmobile.comwrote: Mark Rowe wrote: On 2009-06-11, at 15:16, Ojan Vafai wrote: On Sat, Jun 6, 2009 at 7:14 PM, Ojan Vafai o...@google.com mailto: o...@google.com wrote: On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com mailto:mr...@apple.com wrote: There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) It seems to me that all the issues raised with using reitveld are solvable. Assuming you all are OK with doing this iteratively instead of needing all the issues to be resolved on day one, I think we can probably start taking concrete steps forward. Given what has been said so far I'm still not clear why Rietvald is a better option than Review Board. Well, I haven't heard anything concrete on why Review Board is better than rveld, either. All I've seen are some posts saying, You know, Review Board exists too. Is the UI better? Is the architecture better? Is the design very different? Is it easier to integrate with git? Exactly how much work is involved in hosting each of these solutions? The only thing concrete I've seen is that Review Board is self-hosting, while rveld is tied to AppEngine. ...and there's some that you could run rietveld without AppEngine infastructure if necessary. er...there's there's some _evidence_ that you could run rietveld without AppEngine infastructure if necessary. (referring back to Ojan's comment) FWIW, another project I (used to) work on tried Review Board out about a year ago. We found it to be pretty clunky, so we continued just doing code reviews via diffs in email. Maybe it's improved since then. Either way, I believe the team is now using Rietveld for large reviews. J ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
Mark Rowe wrote: - UI is intimidating to newcomers. This is clearly subjective, but since the goal here is to make the review process friendlier, especially for new contributors, I think it deserves calling out. FWIW, after playing around with it for a few minutes I find its UI much, much friendlier than Bugzilla's itself. However, add me to the list for not unless we can get seamless integration with the bug tracker and source control. I think the biggest confusion for newcomers would be keeping track of the difference between the three tools, not learning how to use each one. It doesn't matter to me whether this is achieved by actual deep integration with Bugzilla, or just passing data back and forth, as long as it feels like deep integration to the user. As a wild speculation: how hard would it be to build a new bug tracker by adding fields to Rietveld's issue descriptions, rather than trying to make changes to Bugzilla? (A new bug report would simply be an issue without any patches uploaded yet. We would need a space for general comments not tied to a line of code. We'd need more metadata about each issue (component, priority, etc) and probably some new search and summary screens. What else would be needed?) Add a CON for Rietveld: there's a surprising amount of overlap between computer terminology and the Rietveld method of crystallography, making it hard to sort out google results when looking for reviews of it. Does anybody know of any unbiased third-party user stories that might help us evaluate the tool? Ojan or others who know the tool - can you explain a bit more about how it integrates with svn? I was under the impression that you just attach a patch, which could be from any source, but browsing http://code.google.com/p/rietveld/issues/list shows a few bug reports about svn integration that make it seem Rietveld is pulling more data from it (for instance, issue 82: ignores files created by svn cp, issue 100: fix for upload.py and svn with externals, and of course the eloquent issue 117: support cvs). Would git integration mainly be a matter of making sure it parses git's patch output correctly? Subjective, less important issues: - I'm not sure about keeping patches and the bugs that they address in separate systems. It seems that discussion about a bug can end up split between the two systems. I don't think that's a minor issue at all. - It's hard to spell. Retyping it to fix the spelling makes me sad. Agreed. I expect will all end up calling it rfield soon enough (and I even typed that as rfiled the first time). Ojan also mentioned ReviewBoard in his original email. I've used it only briefly, but I do know that it addresses some, but not all, of the issues above (It's not tied to AppEngine, it works with both Subversion and Git, and has some support for external authentication mechanisms). It may address others, but I've not looked closely enough to know for sure. I'd like to hear some more comments on other code review systems. Does anyone have any more in-depth comparisons with rfield? Do they all use basically the same methodology with slightly different UI's, or are there major differences in approach? Joe ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
Joe Mason wrote: Agreed. I expect will all end up calling it rfield soon enough (and I even typed that as rfiled the first time). I mean rveld, of course. I've been rereading Dracula, I think it's affecting my brain... Joe ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On Wed, Jun 10, 2009 at 8:12 AM, Joe Mason joe.ma...@torchmobile.comwrote: Mark Rowe wrote: - UI is intimidating to newcomers. This is clearly subjective, but since the goal here is to make the review process friendlier, especially for new contributors, I think it deserves calling out. FWIW, after playing around with it for a few minutes I find its UI much, much friendlier than Bugzilla's itself. However, add me to the list for not unless we can get seamless integration with the bug tracker and source control. I think the biggest confusion for newcomers would be keeping track of the difference between the three tools, not learning how to use each one. It doesn't matter to me whether this is achieved by actual deep integration with Bugzilla, or just passing data back and forth, as long as it feels like deep integration to the user. As a wild speculation: how hard would it be to build a new bug tracker by adding fields to Rietveld's issue descriptions, rather than trying to make changes to Bugzilla? (A new bug report would simply be an issue without any patches uploaded yet. We would need a space for general comments not tied to a line of code. We'd need more metadata about each issue (component, priority, etc) and probably some new search and summary screens. What else would be needed?) Add a CON for Rietveld: there's a surprising amount of overlap between computer terminology and the Rietveld method of crystallography, making it hard to sort out google results when looking for reviews of it. Does anybody know of any unbiased third-party user stories that might help us evaluate the tool? Ojan or others who know the tool - can you explain a bit more about how it integrates with svn? We use a script with Rietveld, gcl.py, that handles changelists. It allows you to have multiple changes on the same repository, each identified by a name. The script automates creation of a Rietveld issue when you first upload. It handles things like copied/moved/deleted/image files. I was under the impression that you just attach a patch, which could be from any source, but browsing http://code.google.com/p/rietveld/issues/list shows a few bug reports about svn integration that make it seem Rietveld is pulling more data from it (for instance, issue 82: ignores files created by svn cp, issue 100: fix for upload.py and svn with externals, and of course the eloquent issue 117: support cvs). Would git integration mainly be a matter of making sure it parses git's patch output correctly? Subjective, less important issues: - I'm not sure about keeping patches and the bugs that they address in separate systems. It seems that discussion about a bug can end up split between the two systems. I don't think that's a minor issue at all. - It's hard to spell. Retyping it to fix the spelling makes me sad. Agreed. I expect will all end up calling it rfield soon enough (and I even typed that as rfiled the first time). Ojan also mentioned ReviewBoard in his original email. I've used it only briefly, but I do know that it addresses some, but not all, of the issues above (It's not tied to AppEngine, it works with both Subversion and Git, and has some support for external authentication mechanisms). It may address others, but I've not looked closely enough to know for sure. I'd like to hear some more comments on other code review systems. Does anyone have any more in-depth comparisons with rfield? Do they all use basically the same methodology with slightly different UI's, or are there major differences in approach? Joe ___ 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
Re: [webkit-dev] to reitveld or not to reitveld
On Mon, Jun 8, 2009 at 5:15 PM, Eric Seidel macd...@gmail.com wrote: At least one person has tried to tie review-board with bugzilla: http://www.visophyte.org/blog/2009/03/20/using-review-board-for-bugzilla-request-queues-reviews/ I expect that we'd have to hack review board to do bugzilla-based authentication. (Just like we'd have to hack rietveld to not run on AppEngine if we used it.) I agree with others that the rietveld authentication being tied to AppEngine and thus Google accounts is a show-stopper for WebKit. I have no interest in creating yet another account and then worrying about whether I'm correclty logged into both bugzilla and webkit.org's google account in order to do patch reviews. Currently I avoid dealing with chromium's tracker/review tool because every time I do I have to log out of my gmail.com google account so that I can log into my chromium.org google account. :( AppEngine apps don't have to use Google Accounts for authentication. Rietveld has its own user-account layer, so it's possible to plug-in different forms of authentication. -eric On Sat, Jun 6, 2009 at 7:44 PM, Ojan Vafaio...@chromium.org wrote: On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com wrote: On 2009-06-06, at 15:02, Peter Kasting wrote: On Sat, Jun 6, 2009 at 1:48 AM, Mark Rowe mr...@apple.com wrote: Of the issues that Ojan mentioned in his original email, I see three that would need to be addressed before we could consider adopting Rietveld: - Currently tied to AppEngine. I don't understand why this is problematic in the least, any more than saying Bugzilla is currently tied to being run by Bugzilla. Why does it matter what the backing implementation of Rietveld is? Primarily due to the two points that you trimmed from my email: Two other major issues jump out at me: - Authentication. This is related to the AppEngine tie-in. - Authorization. Patch reviews need to reflect the access controls on the bugs that they are associated with. There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. As I see it, these are the only non-trivial issues with using rietveld. Things like not working with git are trivial fixes (e.g. git adds a/ and b/ to the paths in the diff that need to be ignored). Also, I really don't believe the intimidating UI is a problem in practice. Newcomers get used to it very quickly. I don't know enough about the rietveld code or appengine to say how difficult it would be to address the authentication/authorization issues. These would be the reason's to consider something like review-board instead. My guess is that the access control bit is doable, but I think you'd ultimately still need to sign in to AppEngine using a Google account. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) Review-board would be considerably less effort than integrating something directly into bugzilla. But rietveld would be less effort than review-board if we can get the above issues addressed since there are a number of people who already know the codebase willing to help out here. It seems worth taking a look at how much work it would be to get review-board setup and integrated with bugzilla. Ojan ___ 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
Re: [webkit-dev] to reitveld or not to reitveld
On Fri, Jun 5, 2009 at 11:43 PM, Mark Rowe mr...@apple.com wrote: Dropping our existing practice of using Bugzilla for patch reviews is one way of addressing this. Folding the more useful features of Rietveld in to Bugzilla to improve Bugzilla-based patch reviews is another. We all seem to be in agreement that the tools involved with reviewing a patch have room for improvement, but I've not seen a compelling reason why the former is a better way forward. If people were really interested in changing, then it would take probably two orders of magnitude less effort to set up a Rietveld instance to associate with Bugzilla (to the degree it currently can associate with the Google Code bug tracker), as compared with improving Bugzilla. The former is basically adding a few URLs to some scripts, the latter is highly nontrivial coding. That seems compelling to me. (Right now it's about 10x easier for me to get a Chromium patch reviewed than a WebKit one just because a single shell command can create a Rietveld issue with my patch and set the description up for me.) This something of a non-sequiter, since it is trivial to create a script to do the same with Bugzilla. I've heard mentions of a git-send-bugzilla script that does most of this already, and I'm sure it could easily be adapted for those preferring SVN. True. Still, I _have_ that script for Chromium, and I don't for WebKit :). PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
This something of a non-sequiter, since it is trivial to create a script to do the same with Bugzilla. I've heard mentions of a git-send-bugzilla script that does most of this already, and I'm sure it could easily be adapted for those preferring SVN. https://bugs.webkit.org/show_bug.cgi?id=25603 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On 2009-06-06, at 00:17, Peter Kasting wrote: On Fri, Jun 5, 2009 at 11:43 PM, Mark Rowe mr...@apple.com wrote: Dropping our existing practice of using Bugzilla for patch reviews is one way of addressing this. Folding the more useful features of Rietveld in to Bugzilla to improve Bugzilla-based patch reviews is another. We all seem to be in agreement that the tools involved with reviewing a patch have room for improvement, but I've not seen a compelling reason why the former is a better way forward. If people were really interested in changing, then it would take probably two orders of magnitude less effort to set up a Rietveld instance to associate with Bugzilla (to the degree it currently can associate with the Google Code bug tracker), as compared with improving Bugzilla. The former is basically adding a few URLs to some scripts, the latter is highly nontrivial coding. That seems compelling to me. Per Ojan's original email it is not as simple as adding a few URLs to some scripts, code changes would be needed to make it suitable for our purposes. Let's try and avoid hyperbole: it makes it difficult to have a reasonable discussion. Of the issues that Ojan mentioned in his original email, I see three that would need to be addressed before we could consider adopting Rietveld: - Currently tied to AppEngine. - Doesn't work with diff's generated by git. - UI is intimidating to newcomers. This is clearly subjective, but since the goal here is to make the review process friendlier, especially for new contributors, I think it deserves calling out. Two other major issues jump out at me: - Authentication. This is related to the AppEngine tie-in. - Authorization. Patch reviews need to reflect the access controls on the bugs that they are associated with. Subjective, less important issues: - I'm not sure about keeping patches and the bugs that they address in separate systems. It seems that discussion about a bug can end up split between the two systems. - It's hard to spell. Retyping it to fix the spelling makes me sad. Ojan also mentioned ReviewBoard in his original email. I've used it only briefly, but I do know that it addresses some, but not all, of the issues above (It's not tied to AppEngine, it works with both Subversion and Git, and has some support for external authentication mechanisms). It may address others, but I've not looked closely enough to know for sure. (Right now it's about 10x easier for me to get a Chromium patch reviewed than a WebKit one just because a single shell command can create a Rietveld issue with my patch and set the description up for me.) This something of a non-sequiter, since it is trivial to create a script to do the same with Bugzilla. I've heard mentions of a git- send-bugzilla script that does most of this already, and I'm sure it could easily be adapted for those preferring SVN. True. Still, I _have_ that script for Chromium, and I don't for WebKit :). In my experience doing something to address a problem tends to be a very effective method of making the problem go away, especially when that something is easy to do. - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
Clarifications: On Sat, Jun 6, 2009 at 3:02 PM, Peter Kasting pkast...@google.com wrote: I'm not trying to be hyperbolic. On the other hand, I could simply be flat-out wrong. Although I did re-read Ojan's email and I don't I see him as saying there'd be a lot of actual coding needed to try Rietveld. - Doesn't work with diff's generated by git. I didn't realize git was formally supported by WebKit. I assume git can generate diff/patch/svn-compatible diffs with some options (I am not a git user). Also, a bunch of Chromium team members use git full-time, so I assume they have in fact already solved this problem. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On Sun, Jun 7, 2009 at 7:51 AM, Mark Rowe mr...@apple.com wrote: On 2009-06-06, at 15:02, Peter Kasting wrote: On Sat, Jun 6, 2009 at 1:48 AM, Mark Rowe mr...@apple.com wrote: Of the issues that Ojan mentioned in his original email, I see three that would need to be addressed before we could consider adopting Rietveld: - Currently tied to AppEngine. I don't understand why this is problematic in the least, any more than saying Bugzilla is currently tied to being run by Bugzilla. Why does it matter what the backing implementation of Rietveld is? Primarily due to the two points that you trimmed from my email: Two other major issues jump out at me: - Authentication. This is related to the AppEngine tie-in. - Authorization. Patch reviews need to reflect the access controls on the bugs that they are associated with. There are also concerns about access to the data store of the application, backup procedures, etc. Our existing servers are well understood in this regard. We've also found in the past that having services spread across different systems causes confusion when something goes wrong, for whatever reason, as it's not clear who to contact to address the problem. As I see it, these are the only non-trivial issues with using rietveld. Things like not working with git are trivial fixes (e.g. git adds a/ and b/ to the paths in the diff that need to be ignored). Also, I really don't believe the intimidating UI is a problem in practice. Newcomers get used to it very quickly. I don't know enough about the rietveld code or appengine to say how difficult it would be to address the authentication/authorization issues. These would be the reason's to consider something like review-board instead. My guess is that the access control bit is doable, but I think you'd ultimately still need to sign in to AppEngine using a Google account. For what it's worth, we've had next to zero maintenance effort go into keeping rietveld running on appengine. As far as I know, it's been pretty much stable and problem free. But I don't actually maintain it, so I can't say that for sure. :) Review-board would be considerably less effort than integrating something directly into bugzilla. But rietveld would be less effort than review-board if we can get the above issues addressed since there are a number of people who already know the codebase willing to help out here. It seems worth taking a look at how much work it would be to get review-board setup and integrated with bugzilla. Ojan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] to reitveld or not to reitveld
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
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
Re: [webkit-dev] to reitveld or not to reitveld
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
Re: [webkit-dev] to reitveld or not to reitveld
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 onekop...@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 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
Re: [webkit-dev] to reitveld or not to reitveld
1. Avoiding uploading a patch twice is always nice. I think either reitveld should upload attachments when they're attached to bugs on bugzilla, as long as the title matches something like +reitveld 2. I guess I missed saying that. 3. David mentioned that feature. 4. A bot to monitor bugzilla emails would be extremely easy, and it gets updates very quickly (not more than 1 minute, as far as I know, but especially quickly if the email address was also on the machine that runs bugs.webkit.org) Darren VanBuren onekop...@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 onekop...@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 onekop...@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 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
Re: [webkit-dev] to reitveld or not to reitveld
Apparently, according to http://code.google.com/p/rietveld/, we've been spelling rietveld wrong. Darren VanBuren onekop...@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 onekop...@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 onekop...@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 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
Re: [webkit-dev] to reitveld or not to reitveld
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 o...@chromium.org To: Darren VanBuren onekop...@gmail.com Cc: David Kilzer ddkil...@webkit.org; Jeremy Orlow jor...@chromium.org; WebKit Development webkit-dev@lists.webkit.org 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 onekop...@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 onekop...@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 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
Re: [webkit-dev] to reitveld or not to reitveld
On 2009-06-05, at 19:02, Darren VanBuren 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. I don't think that duplicating data in two systems and pushing it back and forth via an RPC mechanism is what Dave had in mind when he was speaking of tight integration. We need to *streamline* the process of uploading and reviewing a patch, and having two different ways to do this seems like a large step in the opposite direction. - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
I agree that using RPC is inefficient, and that we don't want to make the review process any more of a pain. We could also try writing our own code review software specifically designed to work with Bugzilla, so that we could run directly in the Bugzilla environment, and we could modify and retrieve bugs without throwing stuff around RPC channels, just by running some calls in the Bugzilla modules. Darren VanBuren onekop...@gmail.com http://oks.tumblr.com/ On Jun 5, 2009, at 8:48 PM, Mark Rowe wrote: On 2009-06-05, at 19:02, Darren VanBuren 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. I don't think that duplicating data in two systems and pushing it back and forth via an RPC mechanism is what Dave had in mind when he was speaking of tight integration. We need to *streamline* the process of uploading and reviewing a patch, and having two different ways to do this seems like a large step in the opposite direction. - Mark PGP.sig Description: This is a digitally signed message part ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] to reitveld or not to reitveld
On Fri, Jun 5, 2009 at 9:13 PM, Darren VanBuren onekop...@gmail.com wrote: I agree that using RPC is inefficient, and that we don't want to make the review process any more of a pain. We could also try writing our own code review software specifically designed to work with Bugzilla, so that we could run directly in the Bugzilla environment, and we could modify and retrieve bugs without throwing stuff around RPC channels, just by running some calls in the Bugzilla modules. FWIW, in Chromium land we do all the patches *solely* on Rietveld, and never touch the bug tracker at all with them. We have tools that auto-update bugs when patches are checked in and can provide handy links back and forth between the tools, and that's enough. I'm not a WebKit reviewer but I was a Mozilla reviewer, which also does things on Bugzilla, and I don't miss the ability to post a patch on a bug at all. There is literally nothing in that workflow that helps me review/land patches more easily, and it's still just as easy to backtrack after the fact and find what got reviewed/landed starting from a bug. So if people who wanted to use Rietveld to do code review didn't have obvious ways to attach those patches to Bugzilla bugs, I'm not sure it would be a big stumbling block. (Right now it's about 10x easier for me to get a Chromium patch reviewed than a WebKit one just because a single shell command can create a Rietveld issue with my patch and set the description up for me.) PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
