How I Review
I handle a lot of code review and code feedback requests these days. However, it’d be great to get more people doing this for a number of reasons:
- More people exposed to more parts of the code base
- More review bandwidth so more work can be checked into the tree
- Less dependence on a small set of people
In order to get more people doing this, it would be good to document to look for, and how to make sure the code is sound. I’m sure every reviewer does things a bit differently, but I’m going to share my process. There are two types of review I do these days: feedback and review.
Feedback
Feedback is pretty simple to do, and I can usually fly though any patch (even large ones) quickly. This isn’t very thorough (in fact, I tend to keep it to general comments), but I look for the following things:
- correct API usage (XPCOM, jsm, whatever)
- internal invariants are not violated
- any new APIs created make sense and aren’t confusing
- code style matches what’s there, or follows the style guide
Review
Review is more important because once a patch gets r+, it can generally land in the tree. Consequentially, I tend to spend a lot more time on any given patch. In addition to all the things I do for a feedback request (which are looked at more closely for a review), I’ll also look at the following:
- evaluate how well tested is the code that is being added/modified. If it isn’t well tested, I’ll generally suggest a set of test cases that I feel are the bare minimum this needs to land.
- evaluate how this might impact other work going on in this area of the codebase
- ensure this doesn’t add any I/O on the GUI thread
- apply the patch and run the tests
- if the patch looks like it might regress performance, ask for the author to verify that it does not
Note that a number of these things may not be done if I know what the patch author has already done to ensure the patch is safe.
sdwilsh Status Update: 2011-05-13
Done:
- Reviewed bug 654732 - Remove WinCE code from storage/* & db/sqlite3/src/Makefile.in
- Reviewed bug 653630 - Negative value for "heap-used/other" in about:memory due to sqlite reporters double-counting some memory
- Reviewed bug 650509 - Other apps can read Firefox profile files
- Reviewed bug 493783 - Fixing warnings in toolkit components (places and commandline)
- Cleaned up bug 584316 (Fix DownloadManager usage of deprecated Storage binding APIs) so it can land.
- Reviewed bug 633274 - nsINavBookmarkObserver: also pass in GUID whenever we pass in an item id
- Pushed lots of stuff to the try server and triggered a bunch of extra talos runs to start establishing a regression range for bug 655930 - Windows chrome seems to be a 25% pageload hit
- Reviewed bug 577721 - allow repositioning of the web console: above, below and out into a panel/window
- Provided feedback on bug 573176 - Implement Site-Specific Privacy Preferences
sdwilsh Status Update: 2011-05-07
Done:
- Reviewed and merged a pull request for compare-talos. To only make one XHR request to load the results instead of many.
- Reviewed bug 451833 - Highlight the domain name in the address bar
- Reviewed bug 113580 - switch uses of getAttribute to hasAttributeValue whenever possible
- Reviewed bug 399998 - some preference dialogs have an access key for close and some don't
- Reviewed bug 558651 - "Check Spelling" should only be showed in context menu when input element is in 'text' or 'search' state
- Reviewed bug 593460 - Tests must properly close the Inspector
- Traveled to node.conf to help talk about spidernode
sdwilsh Status Update: 2011-04-29
Done:
Managed to keep review turn-around time to at most two days this week, with most patches getting reviewed or feedback within one day.
- Reviewed bug 570189 - Throw an error when there are unbound values
- Reviewed bug 652355 - Remove virtual accessor GetURIExternal from Link
- Reviewed bug 633282 - Aero glass glaze effect jumps down when status info overlay appears
- Reviewed bug 472530 - when statement creation fails, dump lastErrorText to stdout in debug builds
- Reviewed bug 619623 - Intermittent toolkit/components/places/tests/cpp/test_IHistory.cpp | Expected true (or '1'), got false (or '0') at line 364
- Reviewed bug 653261 - Add a function to format the date and time displayed in the Download Manager
- Provided feedback on bug 649154 - Implement DOMCrypt 'window.crypt' as a mozilla DOM property
- Provided feedback on bug 564934 - Implement new Download Manager UI for browser
Getting (and Staying) at Review Queue: 0
I’ll assert that one of the biggest pain points of getting a patch in the tree is getting review. This is largely a problem because it can often take a very long time to get that review from the limited set of people who are allowed to review your patch. Even a veteran contributor who knows his way around the system can have his patches sitting in somebody’s review queue for weeks.
I’ve decided I’m going to do something about this by setting a good example, and hope that others follow. For the past two weeks, I’ve been driving my review queue down to zero almost every day. This sounds like a lot of work (especially for anyone who gets a lot of review requests), but I’ve found that in practice, it isn’t so bad. I usually start out every morning by going through all my bugmail before I dig into my days work. I’ve simply added a step after this where I do code reviews until about noon. This doesn’t often take that long as most patches I get aren’t terribly large, aren’t terribly complicated, or there aren’t many to review. If I happen to not get through all of the review requests, I then set aside an hour at the end of my day to go through the rest. In theory, this means my turn-around time for review requests is one business day. In practice, it has come out to be closer to a two or three days due to a number of large patches that I’ve had to review.
You might say “well, that’s easy to do if you don’t get many review requests!” This is very true, but it’s not like I get a small number of requests. Over the past two weeks I’ve done review and feedback requests for 24 bugs, many of which required more than one review iteration. I don’t think that’s a small number of reviews (at least two per day).
I have one caveat with this though; I don’t want to have the quality of my reviews to degrade. For most patches, I’ve found that if I do any more than five or six a day I start to miss things because I’m too mentally drained at that point. If I have some really large patches I’ll review less, and if it is a bunch of small patches, I’ll review more. Sacrificing code quality for the sake of fast turn-around times isn’t ideal, in my opinion.
I would love it if more people tried this out to see if it works for them. The people requesting review from you would really appreciate it!
