Looking For

Introduction

Mozilla has required code review from the beginning, and it is a key factor in our success. Code review helps us produce better code and better products in many ways, from catching bugs early, teaching and mentoring new contributors, and keeping progress going in the right directions. Strong module ownership and strong domain expertise are essential, and we continue to work to improve these capabilities within the project.

Super-review has its roots in the early days of Mozilla, when we needed a core group of hackers to bring even the early module owners up to speed. This group helped improve quality across the board, and acted as a safety net against the tendency of a large group of hackers to move in widely varied directions. Over time, we have learned a lot about how to work fast and effectively, and in many parts of the code we've done away with super-review in favour of deferring to strong module ownership. This has been both a blessing and a curse, and it has become clear over time that there was room to improve the review process across our codebase to get the best of both worlds.

At this point, the super-reviewers group continues to be composed of senior, experienced hackers who can add value across the codebase in some specific ways separate from domain expertise. We have been bouncing around the problem for quite some time, and the conclusion has been to modify the review policy across the board to be a single policy, using super-review where it will have the biggest impact, and no longer using it where it is unnecessary.

Super-review in Mozilla Hg and CVS repositories

Effective immediately, super-review will be required for certain types of changes for _all_ code residing in mozilla-central (Hg) (and all release branches based on mozilla-central) unless explicitly exempted in the Exceptions section below. All code contained in mozilla.org CVS that is part of the Firefox build is also covered by this policy. This includes areas which previously did not require super-review under the previous policy. Super-review is required in addition to a appropriate module owner or peer review in the cases outlined below. This means that one reviewer cannot provide both review and super-review on a single patch.

What needs super-review?

  • Significant architectual refactoring

    All changes that involve significant changes to how code works and interacts must be submitted for super-review. Code design is hard, and getting more input helps us in the areas of maintainability.

  • All changes involving security

    For security bugs, and changes to security models or mechanisms, we want extra experienced eyes to be involved with those changes.

  • Any change to any API or pseudo-API

    APIs are not just XPCOM APIs, but include global JS utility functions and the like. Designing these better up front makes it easier to build things on top of these APIs, and helps us avoid compatibility-killing "API cleanup" down the road.

  • All changes that affect how code modules interact

    Any other changes that fall outside of the above rules, but affect how two or more code modules function, must have super-review.

Exceptions

cls@seawood.org is the autoconf module owner for Unix, OS2, and other gmake-friendly platforms. His review is sufficient to get changes to those files checked in; staff@mozilla.org require no extra review here.

The NSPR module is mature, stable and has been used in Mozilla antecedents for years. Peer and module owner review is sufficient to get those files checked in; staff@mozilla.org require no extra review here.

The JavaScript engine, like NSPR, is a closed CVS partition containing mature code, with dependencies only on another closed CVS partition (NSPR). Module owner review suffices to change JS code.

Changes to module PSM require at least one thorough owner/peer review. If the person requesting the change is neither the owner nor a peer, a second person from the group of {PSM owners, PSM peers, super-reviewers} must approve the change. Changes to UI that is shared amongst all Mozilla applications (e.g. error messages, password prompts, information windows, certificate manager) shall be reviewed/approved by the Mozilla Foundation's User Experience team. Changes to UI in the primary application windows (e.g. Firefox preferences, security button in mail, status bar items) must be reviewed/approved by the respective application UI owners.

In general, a well-owned module may request super-reviewers to be exempt from super-review requirements. Any exempt module that depends only on modules in closed CVS partitions may become a closed CVS partition, so long as there are enough peers who are actually available to help fix build bustage and other unanticipated problems. We do not expect anyone with CVS access to abuse it, but closed partitions help limit the damage done by accidental checkins, as well as highlight strong ownership.

See despot.cgi for more on modules, a.k.a. partitions.

Seeking a super-review?

No matter who you are (yes, these rules apply to module owners and Mozilla super reviewers too), if you're seeking a review, check the following list before mailing a reviewer:

  1. Make sure a bug is on file that describes the symptom and your diagnosis of the cause that the patch treats (or the feature being added, if you are extending Mozilla).
  2. Run the pre-checkin tests, and any other tests that you think your patch should pass.
  3. Attach the patch to the bug using cvs diff -u format, at a minimum (other attachments with more readable diffs may be added, too).
  4. Write a concise description of what the patch does, and why, in a comment on the bug. Avoid a line-by-line description, since if the changes aren't clear, there should already be comments describing the code in the patch.
  5. Get code reviews from the module owner and any peers you can find; the owner should set the review+ patch flag in the bug.
  6. Read through the rules and tips below; double-check and anticipate any relevant ones.

Module owners should have a peer review their patches, and not do self-review and make a wish.

Then pick an appropriate super-reviewer for the patch from the list below. Request review from that person using the patch flags on the bug: set the super-review flag to ? and fill in the reviewer's name.

Meet the super-reviewers

Here are the strong hackers enlisted by mozilla.org for universal code review coverage:

Inverting to index by area, sorting by primary or most-expert reviewer, and aggregating reviewers into useful mailto: links, gives us the following list. Super-review does not require domain expertise (module owners and peers supply that, usually), so the areas below are not pigeon-holes -- you can solicit a super-review from any reviewer on the list, but using area as a guide will get quicker results in the typical case.

browser
mconnor@mozilla.com, vladimir@pobox.com, robert.bugzilla@gmail.com
cache
darin.moz@gmail.com
caps
mrbkap@gmail.com
composer
neil@httl.net
content, layout
dbaron@mozilla.com, roc@ocallahan.org, rbs@maths.uq.edu.au, bzbarsky@mit.edu, jst@mozilla.org (content), peterv@propagandism.org (content), jonas@sicking.cc (content), mats.palmgren@bredband.net (layout), mrbkap@gmail.com (content), Olli.Pettay@gmail.com (content)
directory
dmose@mozilla.org, bugzilla@standard8.plus.com
docshell, webshell
benjamin@smedbergs.us, jst@mozilla.org, darin.moz@gmail.com, bzbarsky@mit.edu, cbiesinger@gmail.com
dom
jst@mozilla.org, peterv@propagandism.org, jonas@sicking.cc, mrbkap@gmail.com, Olli.Pettay@gmail.com
editor
sfraser_bugs@smfr.org
embedding
benjamin@smedbergs.us
event loop
darin.moz@gmail.com
gfx
pavlov@pavlov.net, vladimir@pobox.com, rbs@maths.uq.edu.au, roc@ocallahan.org
htmlparser
jst@mozilla.org, mrbkap@gmail.com
image libraries
pavlov@pavlov.net, tor@acm.org
js
brendan@mozilla.org, shaver@mozilla.org, mrbkap@gmail.com
mailnews
bienvenu@nventure.com, mscott@mozilla.org, seth@sspitzer.org, dmose@mozilla.org, bugzilla@standard8.plus.com
netwerk
cbiesinger@gmail.com, darin.moz@gmail.com, bzbarsky@mit.edu
security
dveditz@mozilla.com, kaie@kuix.de
strings
darin.moz@gmail.com, jag@tty.nl
toolkit
benjamin@smedbergs.us, mconnor@mozilla.com, vladimir@pobox.com, robert.bugzilla@gmail.com
uriloader
cbiesinger@gmail.com, bzbarsky@mit.edu, darin.moz@gmail.com, mscott@mozilla.org
widget
roc@ocallahan.org, pavlov@pavlov.net, mats.palmgren@bredband.net, mikepinkerton@mac.com
xpcom
brendan@mozilla.org, shaver@mozilla.org, benjamin@smedbergs.us
xpconnect
shaver@mozilla.org, brendan@mozilla.org, mrbkap@gmail.com
xpfe
neil@httl.net, jag@tty.nl
xpinstall
dveditz@mozilla.com
xul, xbl
jonas@sicking.cc
catch-all, when in doubt
brendan@mozilla.org, dveditz@mozilla.com

rules and tips

Two venerable documents that mozilla.org commends to all developers are the SeaMonkey Code Reviewer's Guide and the SeaMonkey Engineering Bible. Here is my synthesis of their wisdom, plus other tips and rules proposed and practiced by wise people, into 21 rules and tips, many phrased in the form of a question (this is not an exhaustive list!). In the following, I'll use "patch" to mean "wholly new file" in the case where new code is being contributed.

  1. Check that the module owner and possibly a peer or two have reviewed the patch, as signified by their r=reviewer's-email-address signatures in the bug report. Beyond the module peers, consider whether you need to bring in other experts, or indeed all reviewers@mozilla.org, or possibly even porkjockeys@mozilla.org.
  2. Are the changes in XP code? If so, are they 64-bit clean? Do they follow the C++ portability guide?
  3. Are the changes visible to end users? If so, are they localizable? Do they follow the recommendations in Writing Localizable/Customizable code?
  4. Do the changes contain or affect XUL user interfaces visible to end users? If so, are they accessible? Do they follow the recommendations in Accessible XUL Authoring Guidelines?
  5. Make sure there is a bug on file, with the patch or patches attached.
  6. Feel free to cc: relevant co-reviewers who are not yet reading the bug.
  7. Encourage patch attachers to use cvs diff -u format when generating a patch, and to run cvs diff from the highest common ancestor directory in order to make a single patch file, which can be applied with one patch command.
  8. If the patch contains many tab expansions or other whitespace-only, purely cosmetic fixes, encourage patch submitters to attach a cvs diff -wu version of their patches, in addition to the diff -u format attachment necessary for others to be able to apply the patch. Use the cvs diff -p option too (put diff -pu8 or something similar in your .cvsrc file).
  9. Look at whole code as well as unified or other context diffs. LXR is your friend.
  10. Are there bugs on file for underlying problems being worked-around, rather than fixed-for-good, by the patch?
  11. How was the patch tested? Were pre-checkin tests run? If the change is large and hard to analyze, were the smoketests also run? How about unit tests or suites, such as the JS tests or viewer?
  12. If the patch allocates memory or modifies allocation and free constructs in existing code, or adds, removes, or moves strong references in data structures, has the patch submitter run the Memory Tools and compared to the tinderbox bloat numbers?
  13. If the patch is daunting, have several to many people in the Mozilla community applied it, run with it, and updated the bug with comments, favorable or otherwise? If not, find some patch-buddies.
  14. Does the patch modify lower-level modules in ways that have hard-to-see effects on higher-layer modules, particularly on XUL, XBL, and the like? If so, has the patch submitter demonstrated only good effects across a broad range of higher-layer, especially XUL-based, applications and dialogs?
  15. Does the patch use nsCOMPtr for strong XPCOM references, nsWeakPtr (if appropriate over against a raw C++ interface pointer) for weak XPCOM references, and the latest string technology? Similar questions apply to any ProgIDs or the newer contractids (any code not already switched by rayw@netscape.com, et al., should switch from progids to contractids).
  16. Does the patch make assumptions about reentrancy or thread-safety that are not clearly documented? If so, push back on the patch submitter for comments, discussion in the Mozilla newsgroups, and eventually, documentation.
  17. If the patch defines new XPCOM interfaces, are they declared using XPIDL? If so, are the interfaces or new methods or attributes [scriptable]? If not, are there good reasons why not? In any event, are JavaDoc-style comments, digestable by Doxygen, used well in the interface definition?
  18. What about compatibility? No frozen XPCOM interface should be modified (instead, a new interface should be created). If a non-XPCOM interface is modified, is the change backward compatible? DOM, XUL, XBL, etc. changes may affect orders of magnitude more programmers than C or C++ changes. If the changes are not backward compatible, consider alternatives to the change that preserve existing functionality. If compatibility must be sacrificed, ensure through newsgroup discussions and bug reports that the loss is well-understood and documented.
  19. Does the patch address problems in the code narrowly, when the bad pattern addressed in a spot-fix fashion recurs elsewhere in the file, or in neighboring files? If so, and if the milestone schedule permits, push back on the patch submitter and the module owner to do the right thing everywhere, and file bugs as needed.
  20. If it's time to minimize risk according to the milestone schedule, consider less sweeping changes, if any, that fix the bug or palliate its symptoms. Make sure dependent bugs remain open to track any open issues (rule/tip #8, above).
  21. When reviewing code, verify that the prevailing module style, if any, has been observed ("When in Rome...") in the patch. If the module is a mess, identify an owner and call for him or her to assert a coding style. Messy files are a telltale of poor or non-existent ownership. The Emacs modeline comment at the top of each file should accurately specify the indentation style of the file, indent-tabs-mode should be nil, and patches should tend to eliminate tabs from the file, until only the proper runs of space characters remain.

Finally, when you're willing to stake your reputation by accepting the patch that you've reviewed, put sr=your-email-address in the bug report, so everyone can see your stamp of approval. Be prepared to make mistakes; everyone does. The point is not to be perfect, or to try so hard that you slow patch acceptance to an unacceptable crawl. The point is to get the right eyes, in addition to many eyes, on the code we're all developing.