Code Review FAQ

What is the purpose of code review?

Code review is our basic mechanism for validating the design and implementation of patches. It also helps us maintain a level of consistency in design and implementation practices across the many hackers and among the various modules of Mozilla. We currently have two levels of review, known as "review" and "super-review."

Of course, code review doesn't happen instantaneously, and so there is some latency built into the system. We're always looking for ways to reduce the wait, while simultaneously allowing the reviewers and super-reviewers to do a good chunk of hacking themselves. We don't have a perfect system, we never will. It's still evolving, so let us know if you have suggestions.

Who must review my code?

You must have an approval ("r=[name]") from the module owner or designated "peer" of the module where the code will be checked in. If your code affects several modules, then generally you should have an "r=[name]" from the owner or designated peer of each affected module. We try to be reasonable here, so we don't have an absolute rule on when every module owner must approve. For example, tree wide changes such as a change to a string class or a change to text that is displayed in many modules generally doesn't get reviewed by every module owner.

If your patch affects the user interface of Firefox, you should read "Requesting feedback and ui-review for desktop Firefox front-end changes" to learn how to get that review performed.

You may wish to ask others as well.

What do reviewers look for?

A review is focused on a patch's design, implementation, usefulness in fixing a stated problem, and fit within its module. A reviewer should be someone with domain expertise in the problem area. A reviewer may also utilize other areas of his or her expertise and comment on other possible improvements. There are no inherent limitations on what comments a reviewer might make about improving the code.

Reviewers will probably look at the following areas of the code:

• “goal” review: is the issue being fixed actually a bug? Does the patch fix the fundamental problem?
• API/design review. Because APIs define the interactions between modules, they need special care. Review is especially important to keep APIs balanced and targeted, and not too specific or overdesigned. There are also specific API change rules that must be followed.
• Maintainability review. Code which is unreadable is impossible to maintain. If the reviewer has to ask questions about the purpose of a piece of code, then it is probably not documented well enough. Does the code follow the coding style guide?
• Security review. Does the design use security concepts such as input sanitizers, wrappers, and other techniques? Does this code need additional security testing such as fuzz-testing or static analysis?
• Integration review. Does this code work properly with other modules? Is it localized properly? Does it have server dependencies? Does it have user documentation?
• Testing review. Are there tests for correct function? Are there tests for error conditions and incorrect inputs which could happen during operation?
• License review. Does the code follow the code licensing rules?

What is super-review?

"Super-review" is a name we don't really like. Something like "Infrastructure Review" or "Integration Review" is probably a more accurate name, and doesn't add a false glamour that "super" seems to imply. But "super-review" has been used for a while now, and we're probably stuck with it.

A Super-review focuses on a different range of issues than does a review; it focuses on how a patch fits into the broader Mozilla codebase. This includes topics such as:

• Use of APIs
• Use of XPCOM,
• Adherence to Mozilla's portability guidelines
• Cross-module effects
• Mozilla coding practices

Does a super-reviewer need domain expertise?

No, a super-reviewer does not need to possess domain expertise in the area the patch addresses. We specifically ask super-reviewers to take on super-reviews in areas where they do not have domain expertise. If this doesn't happen, then load balancing becomes much harder. A super-reviewer with specialized domain expertise will feel that s/he has to review all patches in that area - a sure formula for burnout. Also, there will be subject areas where no one has or wants the overall breadth important to do super-reviews.

Of course, super-reviewers may also have domain expertise. If so, so much the better. We hope s/he will use it when super-reviewing, there are no inherent limitations on the range of comments a super-reviewer might make about improving the code. A super-reviewer who has domain expertise can more quickly estimate how complex the review is likely to be, more quickly complete the super-review, and often provide a more comprehensive evaluation of the patch in context. In other words, any Great Chef can help you improve your meal, but if you're doing Creole, a Great Creole Chef is even better.

So domain expertise is an added bonus, but not a requirement. Super-reviewers who want to can note that they are reviewing only for breadth issues, and lack domain expertise.

Why does the Super-Reviewer document associate super-reviewers with subject areas?

Because as noted above, a super-reviewer with domain expertise can often do a quicker and more comprehensive review. So if there are super-reviewers with domain expertise in your area, start there; let's get the deluxe version of super-review when we can.

If there are no SRs with domain expertise, or if those SRs are too busy or too overloaded with super-review requests to help, then try another super-reviewer. Help yourself with a strong reviewer, so the SR can feel comfortable looking for cross-module issues. Associating super-reviewers with subject areas is intended only to provide a starting point. Patches can go to any super-reviewer.

What if a super-reviewer misses things?

We do not expect a super-reviewer to make everything perfect. Some things will be missed. Hopefully the super-reviewer is still learning new things as well; there's always more to know. The super-reviewer does not become responsible for your work by super-reviewing it.

Do I always need to get a review before a super-review?

No, they can come in either order. However, a super-reviewer can decide that s/he would like to see the review first before starting the super-review. For example, a super-reviewer might want to do this if there is a discussion about how to accomplish the goals of the patch, and the patch might change noticeably before completion.

How can I tell the status of reviews and super-reviews?

When a patch has passed review or super-review you'll see "[name]:review+" or "[name]:super-review+" in the attachment table in the bug report. If it has failed review then you'll see a "[name]:review-" or "[name]:super-review-". Most of the time that a reviewer sets a review flag, he will also add a comment to the bug explaining the review.

How fast should I get a response?

If you have asked a specific reviewer for a SR, you should get some sort of response within 24 hours. At the very least, the response should tell you (a) whether that SR will be able to provide the SR; and if so, (b) a timeframe for completion. We recognize that 24 hours can be a long time to wait to hear that a SR cannot do a requested super-review, and we ask the SRs to respond ASAP when they know they can't do a requested SR.

Many patches will also receive substantive comment within the 24 hours, but the SRs have not signed up to give 24 hour turn-around on substantive review. You should also get prompt notification from a SR if he or she later learns that he or she can't do the SR or was too optimistic about the timeframe.

If your patch is small enough and simple enough that you believe a super-review really isn't needed, then note this in your request ("requesting 'rs= '"). Super-reviewers have the discretion to "rubber stamp" a patch. This means that the SR agrees that a patch is so basic that super-review can be skipped. If the SR agrees, a rubber stamp can often be received very quickly.

What if I don't get any answer?

A super-reviewer might drop the ball and not get to the patch. They're not perfect and probably have way too much work to do. Ping them again if you don't hear back in 24 hours. Try another super-reviewer. If you consistently have problem with this, please provide mozilla.org staff with data. (Please do not send complaints without data. "This took me a week" is useless without enough real data to figure out where the problem is.) We should be able to have a much better answer as we get a tracking mechanism in place.

What is the chance of getting an actual super-review within 24 hours?

That depends. Partly on your patch, partly on the super-reviewer's workload. Each super-reviewer works a little differently, so we can't give a template for exactly how reviewers prioritize their coding, super-reviews and other work, or where your patch will fit. Some super-reviewers are more interrupt-driven and will stop their work to do an easy review immediately. Others will try to complete a chunk of their own work without interruption before turning to super-reviews. It's pretty obvious that a patch that is easy to review and fixes a big problem is likely to get attention quickly. Equally obvious, a large patch which requires hours of concentrated focus to review is going to take a while.

Why can't I check in now and get super-review later?

There are a lot of reasons. Here are a few, not necessarily in order of importance.

Super-reviewers find problems that will affect everyone if checked in. It doesn't make sense to make everyone on the trunk suffer for problems that would have been caught by super-review. As Spock would say, 'The good of the many outweighs the good of the few.'

Checking code in seems to cause a psychological effect that the patch is done, it's time to go on to the next thing, and requested changes are new and extra work. It's silly to have people feeling something is done when it's not.

Tracking whether or not a super-reviewer's comments have been implemented is harder if the comments are made after the code is checked in.

What kind of response might a super-reviewer give?

Some typical responses include:

• Give an "sr=[name]."
• Give an "r/sr=[name]." This is possible when the SR is also designated as a reviewer for the affected area. This response means that the approval can be used as either the review or the super-review (but not both). This gives the patch author a larger pool of people from whom the second review can be received.
• Request changes and re-submission of updated patch.
• Request changes and note something like "once changes made, sr=[name]."
• Give an "rs=[name]." This means that the super-reviewer believes the patch is small or obvious enough that a super-review is not needed.
• Ask that the module owner complete that review first.
• Ask someone to apply the patch first and test it (often a good teaching device).
• Ask another super-reviewer to take the patch.
• Ask for clarification, or a risk analysis.

Super-reviewers may make suggestions not on this list. We hope they do, we certainly don't have all the answers today.

Why do I need to wait for a super-reviewer to give a "rs="? Why can't I make the decision that this is a minor change not needing super-review myself?

Because it's easier to develop and maintain consistency among a group of 25 people than it is among all those who have check-in access. If you think your patch belongs in the "rs" world, note this in your request. This might encourage a super-reviewer who has a small window of time to look at this first. Of course, a SR is always free to disagree and determine that the patch really does need super-review. And the "rs wanted" notation will not carry much weight if you put this in all your patches and develop a reputation for "crying wolf" in reverse.

Why can't a SR give both an R and a SR if he's the module owner?

Because we want two sets of eyes on code that goes into the tree.

What if one or more reviewers say my patch is low priority and they won't get to it for a while?

Well, they may be right, painful though it is to get such a response. Try another super-reviewer.

Can the super-reviewer ask me to do something that is not already precisely specified in the bug?

Yes, the SR has discretion here. We do want to be continuously upgrading code quality, and making auxiliary changes related to the patch may be the right thing to do. On the other hand, SRs do not have carte blanche. For example, it would probably be excessive to require a complete update of the strings implementation of a module in order to fix typos. There should be some reasonable relation between the scope of the bug identified and the scope of change requested by the SR.

What if the super-reviewer asks for a change that seems way out of scope to me, like the strings example above?

Ask the SR if his or her request can be moved to a new bug, which can be evaluated, prioritized and fixed on its own. And whether your change can be checked before this new bug is fixed. If you and the SR can't reach agreement on this, go to another super-reviewer and/or [email protected] for arbitration. Be sure to say that you are having this difference of opinion; asking for another super-review and hoping for a more palatable response from the second reviewer will lead to more problems.

Do the Super-Reviewers look at the User Interface / User Experience issues as well?

Yes. Currently we do not have a separate "UI/UE review" before code is checked into the tree. It's been proposed once or twice, but we're not eager to institute another level or review. Instead, we ask the SRs to give some thought to UI issues, and note if they see something that (a) appears to break the UI; (b) is inconsistent with other UI elements or with the specification; (c) makes significant UI changes without citing a specification; or (d) otherwise appears odd.

In the case of (a) SRs would ask that this get fixed as part of a super-review. In the case of (b), (c) or (d) we expect the SRs to ask that the UI folks be involved. We do not expect the super-reviewers themselves to make UI decisions based on personal preference.

What if the SR requested changes that I don't understand or don't know how to implement?

The SR is responsible for making a clear request, but is not responsible for teaching you how to correct your code. If it's time to do some learning to implement the requests, follows the usual channels. Ask people you know in the community, your work colleagues, ask in IRC and the newsgroups. If you're employed to work on Mozilla, your company may have additional training or mentoring techniques.

Is super-review intended as a training mechanism?

Yes and no. SR is a good mechanism for intermediate to advanced training. On the other hand, SR is a terrible mechanism for training in basic practices. Basic training needs to be spread very deeply throughout the community. The SRs are too few and too busy to provide this level of 1-1 training for any length of time. Asking this is another sure way to cause burnout. So we want to find other mechanisms for training, and have super-review be the last stop for training.

What if I want to be a super-reviewer?

Search your heart. Are you sure you want to be an infrastructure reviewer, and aren't being lured by the idea that "super" means something more glamorous than the core plumbing of the product? If you still want to be a super-reviewer, then find a way to demonstrate your facility with the areas that super-reviews cover. If you are already doing code reviews, expand the scope to include super-review items. Or maybe one of the super-reviewers would like an assistant.

You'll want to show the super-reviewers that:

• You write code that demonstrates understanding of infrastructure and integration issues.
• You provide good, careful reviews
• Your code reviews identify infrastructure and integration issues.
• Your feedback provides clear comments, demonstrates teaching skills regarding infrastructure and integration issues.
• You communicate well
• You have general respect in the Mozilla community

When you think you are ready, find a super-reviewer who is willing to propose you to the SRs as a group. A super-reviewer might be willing to do this because s/he believes you are ready, or close enough to ready that it makes sense to have the discussion. We would not expect a super-reviewer to propose someone s/he felt was clearly not ready.

When there's a consensus among the super-reviewers that you're ready, you can join the group. We haven't formally defined "consensus" or the process for documenting consensus yet; we'd like to proceed on an ad hoc basis for now. Let's add process when we need it, and not before.