Re: Code Review Session
On 2013-05-31, at 15:46 , Boris Zbarsky bzbar...@mit.edu wrote: On 5/24/13 10:50 AM, Justin Lebar wrote: Consider for example how much better harthur's fileit and dashboard tools [1] [2] are than bugzilla's built-in equivalents. Wasn't fileit integrated into the New Bug page? That search suggestion drop-down was a relatively recent addition. [2] http://harthur.github.io/bugzilla-todos/ So I actually tried using the dashboard you link to for the last week. It's actually worse at least for my use case than what I do in bugzilla right now (which is just a saved search for review/feedback requests on me), because: 1) It does not show feedback requests. This may explain why some people are routinely ignoring them…. Good point. I filed: https://github.com/harthur/bugzilla-todos/issues/19 2) It's a lot slower to load than a saved search. With my current request queue it didn't seem that much slower. 3) If left open (see #2) it does a bunch of JS off timeouts, causing noticeable jank in my browser. It polls so you can see new requests come in in real-time. One of the nice features is if you have it in an app tab, the favicon grows a little badge showing you the number of new requests you have. Of course, this requires some round-tripping with the server. All of which is to say that use cases apparently differ, since I assume for you the bugzilla-todos dashboard is in fact a lot better. Of course I would love something that did what this dashboard does in terms of showing bugs to check in and whatnot without the drawbacks. ;) Fork and create! ;) I tend to use a mix of bugmotodo and the Bugzilla Dashboard. I find them both useful, though lately I've been using bugmotodo in an app tab and quite like it. As long as I can see review requests in a timely fashion, it's all good. :) ~ rob ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
self-reply. --1. On 2013-07-11, at 09:56 , Rob Campbell rcampb...@mozilla.com wrote: On 2013-05-31, at 15:46 , Boris Zbarsky bzbar...@mit.edu wrote: […] 1) It does not show feedback requests. This may explain why some people are routinely ignoring them…. Good point. I filed: https://github.com/harthur/bugzilla-todos/issues/19 This issue is closed. Apparently a more-recent version of bugmotodo now includes feedback requests in the To Review pane. ~ rob ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On Friday, May 24, 2013 8:05:31 AM UTC-7, Mike Conley wrote: Sounds like we're talking about code review. But I want to qualify integration into bugzilla: I explicitly do not want a tool that is tightly coupled to bugzilla. In fact, I want a tool that has as little to do with bugzilla as feasible. I'm a contributor to the Review Board project[1], which is not coupled with Bugzilla whatsoever. This quarter, we've dug into looking at upgrading the review experience in Bugzilla. And after wrestling some more with splinter and webkit's code-review.js tool, we decided that review board is a whole lot better approach. We're working with IT to stand up a review board server that Bugzilla will make use of, so it won't be tightly coupled (as I understand it). Cc'ing Glob and Mcote who can provide more details. It also has an extension called ReviewBot[2], which can run patches through static analysis or automated tests, and inject the results automatically into the review request as a ReviewBot review. That sounds like something else we should investigate in addition to Review Board. Cheers, Clint ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/28/13 7:39 AM, Benjamin Smedberg wrote: Bugzilla is our tracking tool of record. I'm personally rather bullish about bugzilla improvements, now that the 4.2 upgrade is done and we have solid people working on it and making weekly improvements. Yeah. It has its share of flaws, but it's also battle-tested, and has had significant recent improvements recently. I'd also given up on it in the past, but it's on a better path now. Multiple tools end up being confusing, and reliance on external tools carries risk that if they shut down we'll lose important history. (This is already Not Fun when spelunking in old Mozilla code, and finding that something landed without any bug number to explain what it was doing or why, nor context of the thinking or issues that let to the change.) I think it's fine (good, even!) to have small / experimental projects try new things, but the expectation should be that once those projects become non-experimental / production, they should return to the usual tools. (And we should be improving / expanding the usual tools to meet modern requirements.) Justin ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/23/2013 5:32 PM, Scott Johnson wrote: Members of dev-platform: As part of the Web Rendering work-week in Taiwan, we had a discussion of the process of code review, graciously led by roc. If you were unable to attend, or were able to attend and would like to review the proceedings, notes are available here: https://etherpad.mozilla.org/TaiwanWorkWeekCodeReview Special thanks to Anne van Kesteren and Daniel Holbert for assisting me in the note-taking when my laptop battery died. ~Scott Video of the session is available at: http://people.mozilla.org/~cpearce/CodeReviews-Taipei-May2013.mp4 (1 hour 10mins duration, 346MB) Chris P. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/29/2013 1:50 PM, Benoit Girard wrote: The clang-format list tells me that there are near-term plans for a standalone clang-tidy utility built on clang-format that will do much of what we're looking for as far as basic code-cleanup goes. I'm asking around about what near-term means, and if the answer isn't good enough I'm going to try to add something to clang-format to give users some moz-style guidance as a temporary measure. I'd be happy to replace what's I'll be rolling out in bug 875605 once something better comes along. But c++ isn't new so I'm not holding my breath :). From: Daniel Jasper djasper-hpiqsd4aklfqt0dzr+a...@public.gmane.org Subject: [PATCH] Initial clang-tidy architecture Date: Wed, 29 May 2013 03:30:35 -0700 I call that near-term. :-) -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/24/13 10:50 AM, Justin Lebar wrote: Consider for example how much better harthur's fileit and dashboard tools [1] [2] are than bugzilla's built-in equivalents. [2] http://harthur.github.io/bugzilla-todos/ So I actually tried using the dashboard you link to for the last week. It's actually worse at least for my use case than what I do in bugzilla right now (which is just a saved search for review/feedback requests on me), because: 1) It does not show feedback requests. This may explain why some people are routinely ignoring them 2) It's a lot slower to load than a saved search. 3) If left open (see #2) it does a bunch of JS off timeouts, causing noticeable jank in my browser. All of which is to say that use cases apparently differ, since I assume for you the bugzilla-todos dashboard is in fact a lot better. Of course I would love something that did what this dashboard does in terms of showing bugs to check in and whatnot without the drawbacks. ;) We shouldn't conflate owning the PR data with integrating the PR tool into bugzilla. I think that depends on what integrating means. Ideally, we should have something that makes it easy, starting from a line of code to track back why that line of code is the way it is. In a perfect world that would mean correct blame, with the changeset linking to the discussion about the patch, reviews, design documents if they exist, etc, etc. We're not going to get there, if nothing else because a lot of that seems to be in email/irc/hallways, but the closer we can get the better. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/24/13 8:46 AM, Benoit Girard wrote: I've got some patches that import webkit's check-style script to check the style[1]. Google and Linux also have style lint scripts (cpplint.py [1] and checkstyle.pl [2] respectively) that don't depend on a particular compiler tool like clang-format. [1] https://google-styleguide.googlecode.com/svn/trunk/cpplint/cpplint.py [2] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl cpeterson ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On Mon, May 27, 2013 at 10:54 PM, Anthony Jones ajo...@mozilla.com wrote: A pre-upload check would give the fastest feedback. I'll be checking in a script in the mozilla repo that can be ran offline and produce the same results. On Tue, May 28, 2013 at 10:44 AM, Mike Hoye mh...@mozilla.com wrote: On 2013-05-27 10:54 PM, Anthony Jones wrote: A pre-upload check would give the fastest feedback. It would help me (and those who review my code) if there is an easy way to check my patches from the command line before doing hg bzexport. Even if it is only for white space. What we need is a way to specify which file paths the standard formatting rules apply to. I just need to figure out how to install clang-format. The clang-format list tells me that there are near-term plans for a standalone clang-tidy utility built on clang-format that will do much of what we're looking for as far as basic code-cleanup goes. I'm asking around about what near-term means, and if the answer isn't good enough I'm going to try to add something to clang-format to give users some moz-style guidance as a temporary measure. I'd be happy to replace what's I'll be rolling out in bug 875605 once something better comes along. But c++ isn't new so I'm not holding my breath :). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/24/2013 10:50 AM, Justin Lebar wrote: * I think we should experiment (again) with real pull-request integration into bugzilla. I'm totally in favor of better tools and real pull requests, and of course the PRs need to be linked to bugzilla /somehow/. But I want to qualify integration into bugzilla: I explicitly do not want a tool that is tightly coupled to bugzilla. In fact, I want a tool that has as little to do with bugzilla as feasible. I wasn't going to reply to this, but the more I think about it the more I'm afraid I have to. Bugzilla is our tracking tool of record. I'm personally rather bullish about bugzilla improvements, now that the 4.2 upgrade is done and we have solid people working on it and making weekly improvements. But even if that weren't the case, I don't see how we can effectively separate the two functions. I've worked on projects which did that (Breakpad), and what happened was that people stopped using the issue tracker and it got hopelessly out of date. New contributors couldn't figure out the status of issues, and the cc lists for the review system and the bug system were completely separate. We know we need to integrate the review/PR system with bugzilla enough to preserve security properties and security groups. I'm also convinced that we should keep email notifications controlled by bugzilla cc's; we need to keep the two systems close enough together that the risk of forking bugs and reviews is low. [1] http://harthur.github.com/fileit/ I don't see how this tool is in any way better than the search box at the top of https://bugzilla.mozilla.org/enter_bug.cgi?full=1 [2] http://harthur.github.io/bugzilla-todos/ And I don't see what this example has to do with the rest of the discussion. Bugzilla explicitly tries to make it easy to do your own dashboards via both XMLRPC and the REST API, and many people have made their own flavor of awesome dashboards. But that doesn't really affect anything about requirements in the review process, does it? --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 25/05/13 04:16, Ehsan Akhgari wrote: On 2013-05-24 11:46 AM, Benoit Girard wrote: Another option is to use clang-format, which can lexically parse diff files. A pre-upload check would give the fastest feedback. It would help me (and those who review my code) if there is an easy way to check my patches from the command line before doing hg bzexport. Even if it is only for white space. What we need is a way to specify which file paths the standard formatting rules apply to. I just need to figure out how to install clang-format. If this proves to be successful then we could consider putting effort into other style conventions. Anthony ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
This is only tangentially on topic, but I have a git pre-commit hook which detects .orig files and trailing whitespace. It's saved me a lot of embarrassment. I also have a git tool which will fix trailing whitespace in your patch. https://github.com/jlebar/moz-git-tools#pre-commit https://github.com/jlebar/moz-git-tools#git-fix-whitespace On Mon, May 27, 2013 at 10:54 PM, Anthony Jones ajo...@mozilla.com wrote: On 25/05/13 04:16, Ehsan Akhgari wrote: On 2013-05-24 11:46 AM, Benoit Girard wrote: Another option is to use clang-format, which can lexically parse diff files. A pre-upload check would give the fastest feedback. It would help me (and those who review my code) if there is an easy way to check my patches from the command line before doing hg bzexport. Even if it is only for white space. What we need is a way to specify which file paths the standard formatting rules apply to. I just need to figure out how to install clang-format. If this proves to be successful then we could consider putting effort into other style conventions. Anthony ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
wow, looks like is missed quite a lot while my power was out.. a high level response from the bmo team is: a goal for this quarter is to address a lot of the review related issues. i've been working through splinter issues and fixing the easy ones (it no longer has problems with renames/copies!), and have also been scoping out the viability of replacing splinter with either review-board or webkit's system. should review-board be viable (and i hope it will be), the plan is to host a customised version of review-board, tightly integrated with bugzilla. i have a preliminary sounds reasonable from IT with regards to hosting, and i'm waiting on word from our security team before proceeding with a more detailed scoping exercise (bug 874767). Justin Lebar wrote: I mean no disrespect to our bugzilla maintainers, who have an impossible and largely thankless job, but bugzilla has so much baggage from the '90s, my experience is that it ruins everything it touches. We shouldn't conflate owning the PR data with integrating the PR tool into bugzilla. If we do, we risk ending up with yet another crappy non-solution to a real problem (see bugzilla interdiff, splinter integration, and so on). it's very hard to not take disrespect with you saying that we ruin everything we touch and turn it to crap. there needs to be integration between review-board and bugzilla from a security point of view (patches on secure bugs need to stay secured), as well as from a process perspective (the results of a review should be emailed to everyone involved in the bug, and the bug needs to be updated in some way). to me the way to achieve this is to continue to use bugzilla as the source of truth (ie. attach the patch to the bug), but conduct the reviews in review-board with automatic updating of the bug. -byron -- byron - irc:glob - bugzilla.mozilla.org team - ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 5/23/2013 5:32 AM, Scott Johnson wrote: Members of dev-platform: As part of the Web Rendering work-week in Taiwan, we had a discussion of the process of code review, graciously led by roc. If you were unable to attend, or were able to attend and would like to review the proceedings, notes are available here: https://etherpad.mozilla.org/TaiwanWorkWeekCodeReview Special thanks to Anne van Kesteren and Daniel Holbert for assisting me in the note-taking when my laptop battery died. I'll reply here, but I'm not sure if there are other places we should post this information: * patch size/patch contents included in review email: this was mostly fixed in bug 689601 * Automated tools: mhoye has identified lack of automated review as one of our biggest blockers to getting more mentors involved and having successful mentoring for new volunteers. It turns out that nobody wants to mentor bugs when most of the interaction involves please fix this whitespace/style/etc. cc'ing him so he can provide more details. * I think we should experiment (again) with real pull-request integration into bugzilla. I've been looking at gerrit, which shows some promise but may be very difficult to integrate. http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation resonates strongly with me personally, but I don't know if the actual tool will be good enough to use without major modifications. There are certainly some valuable things we could do with pull requests, such as direct integration with the tryserver and automatic pushing after review. --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
Sounds like we're talking about code review. But I want to qualify integration into bugzilla: I explicitly do not want a tool that is tightly coupled to bugzilla. In fact, I want a tool that has as little to do with bugzilla as feasible. I'm a contributor to the Review Board project[1], which is not coupled with Bugzilla whatsoever. It also has an extension called ReviewBot[2], which can run patches through static analysis or automated tests, and inject the results automatically into the review request as a ReviewBot review. It has support for teams / review groups, and multiple repositories (both private/public Github repos, as well as self-hosted hg repos). I'm happy to answer questions about Review Board if anybody has any. If we're talking about considering new review tools, I think Review Board should be on the list of things to try. Here are some pretty pictures: http://www.reviewboard.org/screenshots/ -Mike [1]: http://www.reviewboard.org/ [2]: https://github.com/smacleod/ReviewBot On 24/05/2013 10:50 AM, Justin Lebar wrote: * I think we should experiment (again) with real pull-request integration into bugzilla. I'm totally in favor of better tools and real pull requests, and of course the PRs need to be linked to bugzilla /somehow/. But I want to qualify integration into bugzilla: I explicitly do not want a tool that is tightly coupled to bugzilla. In fact, I want a tool that has as little to do with bugzilla as feasible. I mean no disrespect to our bugzilla maintainers, who have an impossible and largely thankless job, but bugzilla has so much baggage from the '90s, my experience is that it ruins everything it touches. Consider for example how much better harthur's fileit and dashboard tools [1] [2] are than bugzilla's built-in equivalents. We shouldn't conflate owning the PR data with integrating the PR tool into bugzilla. If we do, we risk ending up with yet another crappy non-solution to a real problem (see bugzilla interdiff, splinter integration, and so on). -Justin [1] http://harthur.github.com/fileit/ [2] http://harthur.github.io/bugzilla-todos/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On Fri, May 24, 2013 at 10:50 PM, Justin Lebar justin.le...@gmail.comwrote: If we do, we risk ending up with yet another crappy non-solution to a real problem (see bugzilla interdiff, splinter integration, and so on). I think that's quite unfair to the people who integrated Splinter. It's not everything I want, but I like it a lot better than what we had before. Rob -- q“qIqfq qyqoquq qlqoqvqeq qtqhqoqsqeq qwqhqoq qlqoqvqeq qyqoquq,q qwqhqaqtq qcqrqeqdqiqtq qiqsq qtqhqaqtq qtqoq qyqoquq?q qEqvqeqnq qsqiqnqnqeqrqsq qlqoqvqeq qtqhqoqsqeq qwqhqoq qlqoqvqeq qtqhqeqmq.q qAqnqdq qiqfq qyqoquq qdqoq qgqoqoqdq qtqoq qtqhqoqsqeq qwqhqoq qaqrqeq qgqoqoqdq qtqoq qyqoquq,q qwqhqaqtq qcqrqeqdqiqtq qiqsq qtqhqaqtq qtqoq qyqoquq?q qEqvqeqnq qsqiqnqnqeqrqsq qdqoq qtqhqaqtq.q ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On Fri, May 24, 2013 at 08:40:29AM -0700, Michael Hoye wrote: - Original Message - From: Benjamin Smedberg benja...@smedbergs.us To: Scott Johnson sjohn...@mozilla.com Cc: dev-platform@lists.mozilla.org, Michael Hoye mh...@mozilla.com * Automated tools: mhoye has identified lack of automated review as one of our biggest blockers to getting more mentors involved and having successful mentoring for new volunteers. It turns out that nobody wants to mentor bugs when most of the interaction involves please fix this whitespace/style/etc. cc'ing him so he can provide more details. Yeah, so, about that. Having spoken to a handful of developers, this is #2 on the List Of Things People Dislike About Mentoring, having to go back-and-forth with a contributor about style conventions and whitespace. In short, everyone hates it, everyone understands that this should be completely and utterly automated. A question I'm trying to clear up for myself is: I understand that clang-format is somehow inadequate to our needs, but I see that there's an explicit clang-format -style=Mozilla option that claims to be doing the right Mozilla thing. So, I'm hoping somebody can give me a clearer idea of how clang-format is broken. clang-format unfortunately only deals with whitespaces. It does have neat formatting with them, but it's limited to that. Unfortunately, coding style is also about naming conventions (camel case, hungarian notation, etc.), braces positions and presence, etc. Triple-unfortunately, we have an almost infinite number of combinations of those. CELS is a tool that can help with these. https://bitbucket.org/PEConn/cels Mike ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 2013-05-24 11:46 AM, Benoit Girard wrote: On Fri, May 24, 2013 at 10:30 AM, Benjamin Smedberg benja...@smedbergs.uswrote: * Automated tools: mhoye has identified lack of automated review as one of our biggest blockers to getting more mentors involved and having successful mentoring for new volunteers. It turns out that nobody wants to mentor bugs when most of the interaction involves please fix this whitespace/style/etc. cc'ing him so he can provide more details. I've got some patches that import webkit's check-style script to check the style[1]. It just runs regex rather then doing a real parse of the code but importing it turns out to be a low effort/high reward. I've already started working on a robot to post to bugzilla. Perhaps later we can replace it with a more intelligent tool. Another option is to use clang-format, which can lexically parse diff files. Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Code Review Session
On 05/24/2013 11:05 AM, Mike Conley wrote: Sounds like we're talking about code review. But I want to qualify integration into bugzilla: I explicitly do not want a tool that is tightly coupled to bugzilla. In fact, I want a tool that has as little to do with bugzilla as feasible. I'm a contributor to the Review Board project[1], which is not coupled with Bugzilla whatsoever. Noting that although it's not coupled, Review Board makes it very possible to do things that make it an abandon-able solution. Back in 2009 (before there was extension/plugin support) I modified review board so it could pull your review queue out of bugzilla and automatically generate reviews. To address long-term archival needs, I added export functionality that formatted the review as ASCII text that could be copied and pasted into bugzilla. Here's an example excerpt from my blog post at http://www.visophyte.org/blog/2009/06/20/review-board-and-bugzilla-reviews-take-2/: === on file: mailnews/base/src/nsMessenger.cpp line 635 // if we have a mailbox:// url that points to an .eml file, we have to read // the file size as well what a pretty comment on file: mailnews/base/src/nsMessenger.cpp line 642 NS_ENSURE_SUCCESS(rv, rv); please rename rv ARR-VEE === I personally worry about introducing another piece into the existing bugzilla/github interaction, but even 4 years ago, review board had a reviewing experience that is still ahead of both splinter and github. (Although I do have high hopes that github will improve their review experience.) Specifically: - Side-by-side diff (github-only problem) - Syntax highlighted code based on syntax highlighting the entire file, so it's not just a line-centric/keyword centric highlighting. - Expandable context! Don't get stuck with only 3 or 8 lines of context. Expand to see the whole file! - Detects when code is just moved around. This came towards the tail end of my use of it, but it was a killer feature for refactored code. No trying to play the game of match up the added hunk and the removed hunk. It figured it out for you, greatly reducing complexity. Also nice (versus github): - Your review isn't published until you push a button, allowing you to make persistent notes to yourself that you can later remove when you see they aren't an issue without bothering anyone else. This also saves you from proactive people fixing things and pushing fixes as you type them and causing massive confusion. - Reviews don't get nuked by rebases Right now, for tricky reviews, I find myself having to checkout the code locally then use git difftool with the meld 3-way merge tool to diff the branch against its base in order to get syntax-highlighted side-by-side diff with full context and smart intra-line diffing. Andrew ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
