You are currently viewing a snapshot of www.mozilla.org taken on April 21, 2008. Most of this content is highly out of date (some pages haven't been updated since the project began in 1998) and exists for historical purposes only. If there are any pages on this archive site that you think should be added back to www.mozilla.org, please file a bug.

Skip to main content

Mozilla "super-review"

Brendan Eich, Mitchell Baker

mozilla.org requires module owner and peer review before code is checked into the mozilla.org CVS repositiory. Checking into most (but not all) of the Mozilla tree also requires an additional level of pre-check-in code review. This review is done by one or more of a designated group of strong hackers, known as reviewers@mozilla.org. This level of review has become known as "super-review". mozilla.org staff isn't particularly fond of this term, but it seems to be what people use.

Integration review has been used recently instead of super-review. What do people think of this catch-phrase? Mail brendan@mozilla.org with your thoughts.

The goals of super-review are to increase code quality, promote best practices, and reduce regressions. This level of review will look at the quality of the code itself, its potential effects on other areas of the tree, its use of interfaces, and otherwise its adherance to Mozilla coding guidelines. In some cases (cryptography comes to mind), the super-reviewers may not have domain expertise to evaluate the substantive quality of the code. They will still evaluate the other listed factors.

In any case, super-reviewers have discretion to raise "design" as well as "code" red flags, and stipulate remedies, as necessary. This is necessary because mozilla.org does not require a separate design review prior to code review; instead, we expect hackers to talk about their designs, post specs or less formal blurbs about their intentions, and take feedback throughout the (usually non-linear) design/implementation process.

There is a reviewers@mozilla.org mailing list, which has a newsgroup mirror, with the usual, canonically named, mail gateway. This mail/news setup exists so extra reviewers and interested observers can watch requests for review and responses from reviewers (although typically, positive responses from reviewers will be given via bugzilla comments rather than using a message to the newsgroup). Please use this newsgroup/mail-alias to propose changes for super-review, so that interested third parties can observe and help.

Whether or not super-review is required depends on what code you are hacking.

What code needs super-review?

  1. The current rule is: code that is "in-process" or built with the Mozilla browser/mail/news/editor application suite requires super-review.
  2. mozilla.org projects for which super-review is not required include: Webtools (such as Bugzilla), Network Security Services (NSS), Client Customization Kit (CCK) and Rhino. You still need to obtain module owner and peer review, and to comply with any other rules of the particular project.
  3. mozilla.org strongly encourages, but does not absolutely require, super-review for code that is not currently in-process, but that is intended to become in-process. The same is true for code that is not currently built with Mozilla, but which will be in the future. Otherwise, there will be a ton of code to review when you're ready, and it could take eons for the review to be completed. Worse yet, a late review could uncover problems which could have been easily fixed before but which now require significant effort. This category picks up projects such as Personal Security Manager (PSM) which began out of process but which will ultimately be converted to in-process.
  4. Exceptions. Naturally, the basic rules have some exceptions (don't they always?).

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
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)
directory
dmose@mozilla.org
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
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
netwerk
cbiesinger@gmail.com, darin.moz@gmail.com, bzbarsky@mit.edu
strings
darin.moz@gmail.com, jag@tty.nl
toolkit
benjamin@smedbergs.us, mconnor@mozilla.com, vladimir@pobox.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@cruzio.com
xul, xbl
jonas@sicking.cc
catch-all, when in doubt
brendan@mozilla.org, dveditz@cruzio.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.