Development:Reviewing

Patch review in the Camino Project works a bit differently than in the rest of the Mozilla project. This document outlines the review process in Camino and whom to ask for reviews.

How Many Reviews?

Typically, Camino requires three reviews on a patch before it is committed to the cvs repository: two normal reviews and one super-review. This rule can be overridden by any of the super-reviewers (and a second review is not typically required for trivial changes). The reason Camino requires two normal reviews is for greater visibility and to give reviewers a better understanding of more code.

Nibs and other resources

Camino does not currently have the concept of "ui-review" separate from code review, but major changes to UI or interaction are discussed by the team (using mock-ups in bugs or on the wiki), typically early in the feature or patch development. In the event the team is unable to reach a consensus, the final say, as with code, rests with the project leaders and ultimately with the project lead.

Nibs only need a single review, but any significant changes should be discussed thoroughly on the bug and at meetings/on IRC. Bugs that contain only nib or graphics changes (cosmetic bugs) should request a super-review; otherwise, super-reviews on nib or graphics changes are unnecessary.

Attaching a Patch for Review

Before attaching your patch to a bug and requesting review, make sure your patch applies, builds, and works on the current trunk (all development should be done on the trunk and backported to the branches if applicable). For patches which will land on one or more branches, verify that the Mac OS X and/or Gecko functions you use are available.

For instance, while cvs trunk supports only Mac OS X 10.4 and above (and requires the 10.4u SDK and gcc 4.0.1), the MOZILLA_1_8_BRANCH (Camino 1.6.x) must support Mac OS X 10.3.9 and above. Patches that are designed to land on the MOZILLA_1_8_BRANCH (for security releases) and must use Mac OS X functions that are available in the 10.3.9 SDK. Similarly, many Gecko functions available on cvs trunk (Gecko 1.9.0) are either greatly enhanced over their counterparts on the MOZILLA_1_8_BRANCH (Gecko 1.8.1), and many Gecko functions on the trunk do not exist at all on the MOZILLA_1_8_BRANCH.

In any case where a patch is intended for landing on the trunk and the branch(es), you should ensure that the trunk patch will both apply properly and work on the branch(es); if not, you need to supply branch version(s) of the patch.

Once your patch is ready and you have performed the above pre-review checks, attach it to the bug using the “Add an attachment” link on the page, which will bring up the upload attachment/patch page. Give your patch a useful description (perhaps “fix v1.0”) and be sure to tick the “patch” checkbox.

Add a comment if warranted and “take” the bug if it is not already assigned to you.

Then, before pressing the submit button, choose a reviewer as described in Requesting Review below.

Nib changes and other resource files

If your patch requires nib changes, attach any changed nibs in a .zip archive (separate from the actual patch). Also upload a screenshot of the nib as it appears in the compiled app, using Cmd-Shift-4 then space (to get a window-only screenshot).

If you have a small number of resource files (e.g., nibs or images), attach each one separately (images should not be zipped). However, if you have more than about four resources, zip all the resources in a single archive.

Some committers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious.

Requesting Review

Choosing a Reviewer

When requesting review, always request an initial review from one of the reviewers listed below and then, *after* (and only after) receiving review+ from two of them, request super-review from one of the super-reviewers.

It's a good idea to "target" a specific reviewer or super-reviewer; patches set to review? or superreview? with no email address entered in the corresponding Requestee: box tend to get lost. Check the list below to see which reviewer(s) have expertise in the area(s) your patch touches; you will often get a review more quickly that way. You can also check bonsai's cvs log for the file(s) you are hacking and bonsai's cvs blame for the lines of code you are changing to see which reviewers have hacked or reviewed that code before.

Also check the queue (reviews, super-reviews) to see which reviewers are "backed up" before requesting review or super-review; you might also ask on IRC if the reviewer can do a review/super-review first. If you are unsure of whom to ask or have other questions, please ask on #camino on IRC.

Requesting Review on a Patch

To choose a reviewer, while on the upload attachment/patch page in Bugzilla, click on the <select> next to review, set the value to ?, and fill in the desired reviewer in the Requestee text field that appears. Reviewers can be filled in using their Bugzilla email address, personal name displayed in Bugzilla, or any unique subset thereof. To determine the appropriate reviewer for your patch, see below.

(If you’ve already submitted your attachment without requesting review, you can add a review request by clicking on the Edit link next to your patch in the bug's attachment table; the page is similar to the upload attachment/patch page, with the exception of the upload controls and with the addition of a patch viewer.)

The Review Process

Review is typically a process rather than a single event; reviewers will often request changes to your patch before they will approve it. A patch that does not meet with the reviewer’s approval will have its flag set to -, and the reviewer will provide a list of problems or changes in the bug’s comments. A patch receiving review- is common, especially when you are just getting started with Camino. Make the changes the reviewer has requested, post a new patch, and request review again. When the reviewer approves your patch, he will set the ? flag associated with your patch to + and will often leave a comment such as “r=ircnick.“ Soon your patch will be ready for super-review and check-in to the repository.

When you are required to submit a new version of the same patch, be sure to version the filename of the patch and then mirror this version in the patch description when attaching the new patch. For example, a revised version of bugnumber.patch becomes bugnumber_v1.1.patch or bugnumber_v2.patch (depending on the magnitude of the changes) and the patch description in Bugzilla becomes "Fix 1.1" or similar.

On occasion the reviewer will set the the ? flag to + but also leave a comment with changes that need to be made to the patch (usually these are minor changes). When this happens, the reviewer feels that it is not necessary for him to re-review the patch with the changes. Instead, make the changes and post a new patch (obsoleting the old patch via the checkbox on the add attachment page), and set any other flags (a second review?), or set the superreview? flag. (If the changes are very minor, you can proceed to the second review or to super-review without posting a new patch and instead upload a new patch the next time you receive a review- or before check-in if you have received + from all subsequent reviewers and super-reviewers.)

If you have been granted superreview+ with changes requested (often with a line at the end of a review comment like “fix these and sr=pink”), you need only attach a patch with those changes (giving the new patch a description like "Fix 1.1.2, for checkin") and then follow the steps in the Checking In section below.

Your reviewer or super-reviewer may also request, as part of the review comments, that you file follow-up bugs for issues that are out-of-scope for the current patch or which would unnecessarily delay getting the patch into the tree. You should be sure to file these additional bugs (after preparing the final patch for check-in is often a good time to do this).

Reviewers and Owners

Camino doesn't have traditional “module owners” like the rest of the Mozilla project does. However, below is a list of areas of code or functionality in Camino and reviewers/super-reviewers who are comfortable in those areas (items in bold are not formal Bugzilla components).

• Annoyance Blocking:
  • Ad-blocking: ardissone, smorgan
  • Pop-up blocking: murph, smorgan
• Bookmarks: smorgan, cl
• Build Config: mento, ardissone, smorgan
• Cocoa UI: murph
• Downloading: kreeger, smorgan, ilya
• Gecko-related changes/CH-embedding: hwaara, kreeger, smorgan
• History: smorgan
• HTML Form Controls:

  (these bugs are likely Core bugs that should be kicked to Widget:Cocoa in most cases)

• Location Bar & Autocomplete: smorgan, hendy
• Nib changes: ardissone (primarily layout/style and access conformance), smorgan
• OS Integration:
  • AppleScript: peeja
  • Feed handling: kreeger
  • Spell-checking: murph, smorgan
• Page Layout:

  (these bugs are likely Core bugs that should be kicked elsewhere, or bugs in Camino arising from ad-blocking or build-config issues)

• Plugins: smorgan

  (though these bugs are most likely Core bugs that should be kicked elsewhere)

• Product Site: ss, ardissone
• Security: smorgan
• Strings changes: ardissone
• Tabbed Browsing: smorgan, jeff, murph
• Toolbars & Menus: cl
• Translations: marcello

Do not let the list above limit your choice of reviewers; all regular Camino contributors can review any patch (with the exceptions listed below; also, a reviewer may decline to review certain patches due to schedule issues or if the patch is fairly complex and touches code the reviewer is very unfamiliar with).

Exceptions:

• Smokey Ardisson [ardissone] does not review code changes outside of project patches and the areas mentioned above, but will test patches and nit-pick your strings changes.
• Samuel Sidler [ss] also does not review code changes. He should sign-off on all Product Site changes.
• Check with Håkan Waara [hwaara] before requesting review from him.
• Asaf Romano [Mano]'s review is also accepted in Camino, but he is not currently reviewing Camino patches.
• Mark Mentovai [mento] is currently extremely busy and developers should look for alternative reviewers where possible.
• Simon Fraser [smfr] is not currently reviewing or super-reviewing Camino patches, unless you are notified otherwise.
• Mike Pinkerton [pinkerton] is currently only super-reviewing patches.

“Super” Reviews

A super-reviewer can super-review a patch in any part of Camino. There are four people who can give super-reviews in Camino, the three project leads, Mike Pinkerton, Simon Fraser, and Mark Mentovai, and, additionally, Stuart Morgan can be targeted for super-review on any simple or moderate-sized patch with little Gecko impact. (Simon Fraser is not currently reviewing or super-reviewing Camino patches.)

Nib changes associated with code changes do not typically require super-review; reviewers should test the nib thoroughly to make sure it satisfies all the conditions in the Nib changes section above (or request a separate review on the nib from one of our nib specialists). Nib changes taking place without an associated code change should get approval from a super-reviewer, however.

Checking In

After a patch has review+ from two reviewers and superreview+ from one of the Camino leads, it needs to be checked in.

Check-ins for Camino can be made by any of the super-reviewers listed above as well as ardissone, froodian, hwaara and kreeger (note that froodian, hwaara, kreeger, and smfr are not currently checking in Camino patches, nor, with some exceptions, will pinkerton or mento check in patches they have not written).

If no one is around to check in your patch after it has received the requisite approvals, add checkin-needed to the bug's Keyword field and one of the committers should notice and land your patch within a day or so. If they do not, make a comment in the bug or on IRC.

Also note that the mozilla/camino directory hierarchy is open for approved Camino check-ins regardless of the state of various trees and branches, with a few exceptions, such as tagging of major branches, red Camino tinderboxen, missing Camino perf or nightly tinderboxen, etc.

N.B When the Camino tree is closed while the Mozilla tree is closed, there is typically a note on the Camino tinderbox explaining the closure, and the closure is often mentioned in the “topic” in the #camino channel on IRC. However, when the opposite is true—Mozilla is closed and Camino is open—there is typically no indication of the fact. If you are unsure of the state of the Camino tree, ask on in the #camino channel on IRC.

Checkin tips for new (and old) committers

Mercurial

Remember that Hg is a distributed VCS, so that most operations are local. In particular, where “committing” to the official repository was a single operation with CVS (cvs commit), it is a pair of operations with Hg (the local-only hg commit and the local-to-remote hg push).

Configuration changes to allow you to check in

In order to be able to push changes to a remote hg repository, you will need to make a pair of configuration changes:

1. If you do not already have a ~/.ssh/config file to help manage your ssh connections, create the file using your favorite text editor. Once the file exists, insert the following lines:

   Host hg.mozilla.org

User user@domain.tld (where user@domain.tld is the email address associated with your Mozilla Hg/LDAP account)

1. In each local clone from which you wish to push changes (e.g., mozilla-1.9.2 and camino), edit the .hg/hgrc file and add a default-push line. The default-push line should look exactly the same as the default line, except you should replace default’s http:// with ssh://. For example, the .hg/hgrc file for the camino repository should look like this:

   [paths]

default = http://hg.mozilla.org/camino/

default-push = ssh://hg.mozilla.org/camino/

Add a hook to ensure complete checkin messages

For some reason Camino committers appear far more likely to drop a required part of the commit message with Hg commits than we did with CVS. An easy way to prevent bad commit messages is to use a “hook” that checks your commit message for required data and aborts the commit with a helpful error when the data is missing.

Smokey has written a hook that ensures commit messages contain the bug number and at least one reviewer or superreviewer: bugnum. By default the bugnum hook only enforces these commit message rules for mozilla-* and camino, so you can use the script as a global hook and still commit to other Hg repositories that don’t require bug numbers or reviews, or you can customize the hook to add additional repositories to the grep pattern in order to enforce the rules on additional repositories.

1. Install the bugnum script somewhere and make sure that it is executable.
2. Add the following lines to your ~/.hgrc file:

   [hooks]

pretxncommit.bugnum = /path/to/bugnum $HG_NODE (If, for some reason, you don’t want a global hook, you can instead add these lines to the .hg/hgrc file inside each individual repository where you want to use the hook.)

For more about Hg hooks, see Chapter 10. Handling repository events with hooks in the Hg book.

Checking in patches from others

Mercurial has a way of recording the actual patch author even when someone else is committing and pushing the patch. When you’re committing a patch written by someone else, use the following commit command syntax:

hg commit -u "Patch Author <camino.dev@example.org>" -m "Complete commit message" file1 file2

If you find yourself committing patches for someone else frequently, you can add an alias to your ~/.hgrc file to save you some typing:

[alias]
patchGuy = commit -u "Patch Author <camino.dev@example.org>"

Then commit all of Patch Author’s patches using $ hg patchGuy -m "Complete commit message" file1 file2.

Figuring out what patches (commits) you are pushing to the remote repository

Once you’ve committed some patches and are ready to push them to the official repository, you can sanity-check what you’re about to send by using the hg outgoing command. If the list of patches (commits) looks correct, hg push will send them to the remote repository. Be sure to paste the changeset URLs in the bug when resolving it!

CVS

Checking in nibs

Sometimes a person who doesn't have CVS access will post a new or updated nib on a bug that needs to be checked in. The problem with this is that since a nib is technically a directory, there is a CVS folder inside the nib. If you attempt to check in a nib that has been changed by someone who has the CVSROOT set as ":pserver:anonymous@cvs-mirror.mozilla.org:/cvsrooot" (e.g., everyone without cvs write access), you will receive an authorization error from cvs. The problem is that the CVS/Root file is set as the anonymous CVSROOT, not your "user%email.com@cvs.mozilla.org:/cvsroot" CVSROOT.

The easiest way to fix this problem is to copy a Root file from one of the directories you have pulled to and paste it inside the Something.nib/CVS folder.

Alternatively, instead of dropping that person’s nib in place, use cp /path/to/other/nib/*.nib /path/to/your/tree/nib; touch /path/to/your/nib to copy their nib files into your tree.

Trunk/branch mismatches

Sometimes (due mostly to Gecko changes on the trunk) the trunk and branch versions of a patch will diverge and you can't use cross-commit to land both places. If you're confident the branch patch has been well-tested and don't have a branch tree, you can pull just the files you need to commit (rather than pull an entire tree).

1. Set up your cvs environment as normal (i.e., export cvsroot)
2. cvs co -r MOZILLA_1_8_BRANCH -d myfolder mozilla/camino/src/browser/foo mozilla/camino/PreferencePanes/Naviagtion/bar

   where MOZILLA_1_8_BRANCH is the branch tag, myfolder is the folder in which you want the stub tree to live, and foo and bar are the files contained in the patch

3. Apply the patch and commit the files "as normal"

Backing out a patch

Sometimes you accidentally commit something that shouldn't have landed yet, or sometimes (hopefully rarely) you commit something that breaks the tree and needs to be backed out. To back out a patch, follow these steps:

1. If you need to back out something only on the branch and have no branch tree (i.e., you check in using cross-commit), follow the steps above to grab a "stub" branch tree of the files the patch touched.
2. Consult bonsai for the correct backout command(s) to pull files (one per file) to run

   The "Show commands which could be used to back out these changes" link at the top of the appropriate bonsai page will give you the appropriate command, in the form of
   cvs update -j1.17.2.4 -j1.17.2.3 mozilla/camino/src/embedding/CHClickListener.mm
   There will be one command per file (this particular example is a branch file, as you can see from the multiple sub-versions; the current [bad] version of the file is listed first and the previous [good] version is second)

3. touch each file
4. cvs diff -u the file(s) to sanity-check that the bad changes have been reverted in your local copy of the files
5. cvs commit "as normal"; this will commit a new copy of the file(s) with the incorrect change backed out

Adding new files

• cvs add
• cvs add -kb for any binary files, including images, UTF-16-encoded files (e.g., .strings files from external projects), and keyedobjects.nib files inside .nibs.

Adding new nibs

• cvs add the wrapper directory
• cvs add -kb on keyedobjects.nib
• cvs add on the remaining .nib files inside the wrapper directory

[Qalaat-Samaan:trunk/mozilla/camino] smokey% cvs add resources/localized/English.lproj/SafeBrowsingBar.nib
? resources/localized/English.lproj/SafeBrowsingBar.nib/classes.nib
? resources/localized/English.lproj/SafeBrowsingBar.nib/info.nib
? resources/localized/English.lproj/SafeBrowsingBar.nib/keyedobjects.nib
Directory /cvsroot/mozilla/camino/resources/localized/English.lproj/SafeBrowsingBar.nib added to the repository
[Qalaat-Samaan:trunk/mozilla/camino] smokey% cvs add -kb resources/localized/English.lproj/SafeBrowsingBar.nib/keyedobjects.nib
cvs add: scheduling file `resources/localized/English.lproj/SafeBrowsingBar.nib/keyedobjects.nib' for addition
cvs add: use 'cvs commit' to add this file permanently
[Qalaat-Samaan:trunk/mozilla/camino] smokey% cvs add resources/localized/English.lproj/SafeBrowsingBar.nib/classes.nib resources/localized/English.lproj/SafeBrowsingBar.nib/info.nib
cvs add: scheduling file `resources/localized/English.lproj/SafeBrowsingBar.nib/classes.nib' for addition
cvs add: scheduling file `resources/localized/English.lproj/SafeBrowsingBar.nib/info.nib' for addition
cvs add: use 'cvs commit' to add these files permanently