Difference between revisions of "Development:Reviewing"

From Camino Wiki
Jump to navigation Jump to search
(→‎"Super" Reviews: play with grammar; Josh and camino patches)
(→‎Requesting Review: a few more notes)
Line 11: Line 11:
  
 
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).
 
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).
 +
 +
=== Proper patch format ===
 +
 +
Use <tt>cvs diff -u8N</tt> for patches to Camino code.  Diffs should be done from <tt>/mozilla/camino</tt> or <tt>/mozilla</tt> for consistency and ease of application by reviewers and committers.
  
 
=== Localizable.strings ===
 
=== Localizable.strings ===
Line 16: Line 20:
 
mento and smfr prefer clear notes about what strings are added or changed; this hasn't worked great in the past, but neither has attching the file (which can't be diffed, because diff doesn't understand UTF-16).  Use "curly quotes" in the actual strings.
 
mento and smfr prefer clear notes about what strings are added or changed; this hasn't worked great in the past, but neither has attching the file (which can't be diffed, because diff doesn't understand UTF-16).  Use "curly quotes" in the actual strings.
  
=== Project changes ===
+
=== Project (<tt>Camino.xcode</tt>) changes ===
  
(need input from SRs here; this should be part of the patch?)
+
(need input from SRs here; this should be part of the patch, right?)
  
 
=== Nib changes ===
 
=== Nib changes ===

Revision as of 11:16, 15 April 2006

Camino's reviewing is a bit different than that of the Mozilla project. This document outlines how reviewing works in Camino and who to ask for reviews.

How Many Reviews?

Typically, Camino requires three reviews for checkin: 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 from one of the reviewers listed below and then, *after* (and only after) receiving review+ from them, request super-review from one of the super-reviewers.

  • don't sr? with no target; it'll get lost
  • check the sr request queue to see who's backed up and/or ask who can do a sr before targeting your sr?

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

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

mento and smfr prefer clear notes about what strings are added or changed; this hasn't worked great in the past, but neither has attching the file (which can't be diffed, because diff doesn't understand UTF-16). Use "curly quotes" in the actual strings.

Project (Camino.xcode) changes

(need input from SRs here; this should be part of the patch, right?)

Nib changes

Attach any changed nibs in a .zip archive (separate from the actual patch). Some superreviewers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious.

“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.)

Reviewers and Owners

Camino doesn't have traditional "module owners" as the Mozilla project does. However, below is a list of areas 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:
  • Translations: ludo

In addition, initial reviews can be requested from hwaara and Wevah.

Checking In

After a patch has review+ and superreview+, it needs to be checked in. Check-ins in the Camino project can be made by any of the super-reviewers listed above as well as hwaara.

Also note that the mozilla/camino directory is open for approved Camino check-ins regardless of the state of various trees and branches, with few exceptions (tagging of major branches, red Camino tinderboxen, etc.).