I’m going to write something that will probably surprise you. I say this because it sure surprised me when I realized I was even considering what I’m doing a possibility. I’m going to be moving on to something a bit different in the mobile space, and it’s going to be a different kind of challenge for me.
June 1st will be my last day at Mozilla. I’ve learned so much over the years working there, and choosing to leave was the hardest decision I’ve had to make. I do not intend to disappear from the project, however, but my activity level will decrease. Feel free to continue to send review requests my way and cc me to bugs you want my opinion on, and I’ll do my best to reply in a timely manner.
So long, and thanks for all the fish.
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.