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.

Requesting Review

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're hacking and [bonsai's cvs blame] for the lines of code you're 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.

Make sure your patch applies, builds, and works on the current trunk before requesting reviews (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 the 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 the trunk are either greatly enhanced over their counterparts on the MOZILLA_1_8_BRANCH, 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.

To request review, set the review drop-down menu for your patch to ? and then fill in the reviewer's email address in the Requestee field (you can do this when attaching your patch, or afterwards by clicking on the Edit link next to your patch in the bug's attachment table).

Review is typically a process rather than a single event; reviewers will often request changes to your patch before they will approve it (by setting the ? flag to +). 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. Soon your patch will be ready for super-review and check-in to the repository.

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 the review flag on the new patch to + yourself, with a comment like “carrying over r+ from $reviewer”. Also set set any other flags (a second review) to their values from the previous version of the patch, or set the superreview? flag.

Code style

Link to Mozilla coding style guidelines as well as create some for Camino, so we avoid Bug 308942 comment 8 and 14 and Bug 228840 comments 15, 16, 18, and 19.

• Mozilla Coding Style Guide
• License block for new files

Also link to Hacking Mozilla general intro somewhere in our contributor introduction

All new code should conform to Camino's style guidelines, which is a descendent of the Mozilla style guidelines (per decree by our leader):

if (foo) {
  bar;
  blam;
}
else {
  baz;
  zap;
}

Braces for single statements are optional (and discouraged, but some still include them).

We explicitly have no style rule regarding the placement of *. Aim to be consistent within a file in as much as the files are consistent.

Coding for latest-OS-version features or using private APIs

Each situation is unique, but we have provided some existing examples of various strategies employed to correctly code for these situations.

The spell-checker's learnWord: method is undocumented, but is the only way to achieve proper learned spelling for the OS X system dictionary. Example code here and here.

Setting Spotlight metadata on files is also undocumented as of the 10.5 SDK. Our download listener code has a very well-documented and commented example of how to use a method like this safely.

Setting Mac OS X proxy preferences is available only in the 10.4u SDK and later, although the feature is available at least as far back as Mac OS X 10.3.2. PreferenceManager.mm implements this feature safely across all versions Camino supports; see here.

spellcheck learn/add word, wherefrom metadata, quarantine extended metadata, what else?

Situation

Description and mxr link

Proper patch format

Use cvs diff -u8N for patches to Camino code. Diffs should be done from /mozilla/camino or /mozilla for consistency and ease of application by reviewers and committers. All changes—including new source code files and project file changes, if applicable—should be submitted as a single patch (e.g., cvs diff -u8N src/foo/bar src/baz/bam, with the exception of changes to .strings files, nibs, and images; see below for more about requirements for those three file types.

Adding new files in a patch

Sometimes the code you are writing requires the creation of new source files. In order to allow these files to be included in your single patch file, you must perform the additional step of cvs adding the file, or, if you do not have cvs write privileges, of imitating the cvs add process.

To make a patch including a new file (which otherwise requires write access to the repository), use cvsdo to imitate the cvs add operation (alternatively add /Foo.h/0/dummy timestamp// to path/to/file/CVS/Entries), then diff as normal.

To make a patch including files in new directories, first [fill in the details], then diff as normal.

Removing files in a patch

Sometimes your patch obsoletes existing files in the tree.

Instructions TBA

Localizable.strings

Any UI string that appears in the code rather than in a nib needs to have an entry in Localizable.strings (or, in rare instances, a preference pane's Localizable.strings or one of the other .strings files). When writing new strings, always use “curly quotes” in the actual string text.

Beginning with Camino 1.6a1 (23-Oct-2007 nightlies), we have converted all .strings files to UTF-8 .strings.in files which can be diffed and which are then turned into UTF-16 .strings files in the build process. Any changes to .strings.in files should be included in your diff. Any new .strings files should be added to the tree as .strings.in files, converted to UTF-16 .strings files via the STRINGS_FILES mechanism in Makefile.in, and then add the resulting .strings files in generated/ to the project in the appropriate places.

To see string changes, you must rebuild Camino from the command line.

MOZILLA_1_8_BRANCH

Baring any critical security bugs requiring .strings changes, the MOZILLA_1_8_BRANCH is string frozen, and no string changes should occur there.

Project (Camino.xcodeproj) changes

Making project changes has to be done using Xcode (when adding, deleting, or moving files, as well as changing most build/compiler options), specifically when adding files. After making any project changes, simply diff the project file as well and include it in your patch (if you made any changes to build configuration or settings, be sure to diff camino/config/ as well to pick up xcconfig changes). Please check to ensure that your project patches do not change the state of the Camino.app entries in the Camino and CaminoStatic targets (this is usually a problem with srcdir configurations).

(Removing files can be done by hand if you don't have the proper version of Xcode, but this isn't recommended because removing the obvious entries for files will still leave non-obvious entries in the project file and cause crashes/build failures.)

Adding Gecko build products to Camino and the project file

• If the item is "optional" module (e.g., extension) and is not built by default, fix up confvars.sh (on the branch, instead fix up the Camino section of configure.in and regenerate configure) to make Camino pull and build the item.
  • In some cases you can use an ac_add_options flag in your .mozconfig rather than patching confvars.sh/configure.in, but anything that's required to make Camino not burn when all is done needs to be declared in confvars.sh/configure.in.
• If the item required confvars.sh/configure.in or .mozconfig changes, do a full rebuild of your tree.
  • You'll typically have to build both non-static and static unless you're intimately familiar with the location of build products and naming conventions
• If the library or .xpt is something that seems relevant to all embeddors, embedding/config/basebrowser-mac-macho should probably be modified to deliver the build products to dist/Embed/* rather than dist/*
• Add the libraries to appropriate build target(s), and using "Relative to Project" paths when adding
  • In most cases you'll want to add to the non-static (default) and static target separately
  • There are separate folders in the Xcode "sources" tree for static and non-static libs, and for components and "regular" libs
• Adding is best done from a srcdir build, but can be done with an objdir build if you carefully fix paths manually; note that in some cases Xcode will resolve symlinks when adding, so you'll have to fix the paths to point back to dist/Embed/* (or dist/*, if appropriate)
• Move the products from Bundle Resources Copy Phase to the correct copy phase for that sort of Gecko product (for static builds, the static libs belong in the Libraries and Frameworks phase). Different versions of Xcode mis-guess the Copy Phase destinations in different ways.
• Repeat the previous two steps for any .xpt files that the libraries might require
• Check for, and remove if necessary, any bizarre changes to the header or library search paths that Xcode may have “helpfully” added for you.
• Fix nsStaticComponents.cpp for static builds

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). Some committers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious. Nibs only need a single review, but any significant changes should be discussed thoroughly on the bug and on IRC if possible. Bugs that contain only nib changes (cosmetic bugs) should request a super review; otherwise, they are unnecessary.

When editing nibs, be sure to follow the guidelines in Development:Editing Nibs. Until further notice, all nib work should be done only in Interface Builder 2.5.x (preferably IB 2.5.4 under Mac OS X 10.4, as a number of features do not work properly in IB 2.5.6 under Mac OS X 10.5 and its nibs can load more slowly, but it can be run under in a pinch).

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, smfr
  • Pop-up blocking: murph, smorgan
• Bookmarks: smorgan, cl
• Build Config: mento, ardissone
• Cocoa UI: Wevah, murph
• Downloading: kreeger, smorgan
• Gecko-related changes/CH-embedding: hwaara, kreeger, smorgan
• History: smfr, smorgan
• HTML Form Controls:

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

• Input Methods (IME): smfr

  (Camino chrome only; content belongs in Widget:Cocoa)

• Location Bar & Autocomplete: smorgan
• Nib changes: ardissone (primarily layout/style and access conformance), froodian
• OS Integration:
  • AppleScript: smfr, 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: smfr, josh

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

• Product Site: ss, Wevah, ardissone
• Security: smfr, smorgan
• Tabbed Browsing: smorgan, jeff, froodian
• Toolbars & Menus: froodian, 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.
• Samuel Sidler [ss] also does not review code changes. He should sign-off on all Product Site changes.
• Asaf Romano [Mano]'s review is also accepted in Camino, but he is not currently reviewing Camino patches.
• Check with Josh Aas [josh] before requesting review or super-review from him.
• 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

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

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 josh 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.

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 (tagging of major branches, red Camino tinderboxen, etc.).

A tip for 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.

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