Difference between revisions of "Development:Reviewing"
m (→Nib changes: style/grammar police ;)) |
(→Nib changes: explicit FKA) |
||
| Line 43: | Line 43: | ||
=== Nib changes === | === Nib changes === | ||
| − | Attach any changed nibs in a .zip archive (separate from the actual patch). Be sure you do not break the focus and tab cycle when adding or changing controls in nibs. Some super-reviewers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious. | + | Attach any changed nibs in a .zip archive (separate from the actual patch). Be sure you do not break the focus and tab cycle (both normal and Full Keyboard Access) when adding or changing controls in nibs. Some super-reviewers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious. |
When modifying nibs, keep this template in mind (from discussion over {{bug|308942}}). | When modifying nibs, keep this template in mind (from discussion over {{bug|308942}}). | ||
Revision as of 21:57, 26 June 2006
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.
Contents
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. 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).
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.
Also link to Hacking Mozilla general intro somewhere in our contributor introduction
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.
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).
If you make additions or changes to the Localizable.strings file, make a clear note in your comment when attaching your patch, indicating which strings should be added or changed. Do not attempt to diff the file (.strings files are UTF-16, which diff does not understand properly), and do not attach changed Localizable.strings files (which tend to become stale and cause regressions).
Use "curly quotes" in the actual strings.
Project (Camino.xcode) changes
Currently, making project changes needs to happen using Xcode 1.5, specifically when adding files. After making any project changes, simply diff the project file as well and include it in your patch.
Removing files can be done by hand if you don't have Xcode 1.5, 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.
We expect to update to an Xcode 2.1 project file in the near future.
Nib changes
Attach any changed nibs in a .zip archive (separate from the actual patch). Be sure you do not break the focus and tab cycle (both normal and Full Keyboard Access) when adding or changing controls in nibs. Some super-reviewers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious.
When modifying nibs, keep this template in mind (from discussion over Bug 308942).
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).
- Ad-blocking: smokey, smfr
- Bookmarks: smorgan
- Build Config: mento
- Cocoa UI: BruceD
- Downloading: kreeger
- History: smfr, smorgan
- HTML Form Controls: smfr, smokey (forms.css changes), cflawson (forms.css changes)
- forms.css changes require a Mozilla sr (smfr)
- Input Methods (IME): smfr
- Location Bar & Autocomplete:
- Page Layout:
- Plugins: smfr
- Tabbed Browsing: smorgan
- Translations: ludo
Do not let the list above limit your choice of reviewers; all regular Camino contributors can review any patch (though some 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). In addition to the reviewers listed above, initial reviews can be requested from hwaara, Wevah, and jpellico.
“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. A super-reviewer can review a patch in any part of Camino. (Josh Aas is not currently reviewing Camino patches.)
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 hwaara and kreeger.
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 checkin a nib that has been accessed with someone who has the CVSROOT set as ":pserver:anonymous@cvs-mirror.mozilla.org:/cvsrooot", 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.
