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 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 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.