Difference between revisions of "Development:Reviewing"

From Camino Wiki
Jump to navigation Jump to search
(→‎Requesting Review: how to physically do so)
(→‎IRC nick to Bugzilla “unique matches” for Reviewers: move cl to ask and add cpeterso now that he's been around)
 
(95 intermediate revisions by 5 users not shown)
Line 2: Line 2:
  
 
== How Many 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. The reason Camino requires two normal reviews is for greater visibility and to give reviewers a better understanding of more code.
+
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 ==
+
===Nibs and other resources===
−
When requesting review, always request an initial review from one of the reviewers listed [[#Reviewers and Owners|below]] and then, '''*after*''' (and only after) receiving <tt>review+</tt> 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 <tt>review?</tt> or <tt>superreview?</tt> with no email address entered in the corresponding '''Requestee:''' box tend to get lost.  Check the list [[#Reviewers and Owners|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.
+
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 [https://bugzilla.mozilla.org/show_bug.cgi?id=395113 in bugs] or [[Development:Summer_of_Code_2009|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 [[Development:Project_Structure#Project_Leaders|project leaders]] and ultimately with the project lead.
  
−
Also check the queue ([https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=Camino&type=review&requestee=&component=&group=type reviews], [https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=Camino&type=superreview&requestee=&component=&group=type super-reviews]) to see which reviewers are "backed up" before requesting review or super-review; you might also ask on [irc://irc.mozilla.org#camino 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 [irc://irc.mozilla.org#camino #camino] on irc.
+
Nibs only need a single review, but any significant changes should be discussed thoroughly on the bug and at meetings/on [irc://irc.mozilla.org#camino 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.
  
−
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).
+
== Attaching a Patch for Review ==
  
−
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).
+
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<!-- and must compile with gcc 3.3-->.''' 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.
  
−
=== Code style ===
+
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.
  
−
''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.''
+
Add a comment if warranted and “take” the bug if it is not already assigned to you.
  
−
*[http://www.mozilla.org/hacking/mozilla-style-guide.html Mozilla Coding Style Guide]
+
Then, before pressing the submit button, choose a reviewer as described in [[#Requesting Review|Requesting Review]] below.
−
*[http://www.mozilla.org/MPL/boilerplate-1.1/ License block for new files]
 
  
−
''Also link to [http://www.mozilla.org/hacking/ Hacking Mozilla] general intro somewhere in our contributor introduction''
+
===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).
  
−
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 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.  '''Do not include nibs or other binary resource files as part of a git-style Hg patch.'''
−
   
 
−
<pre>
 
−
if (foo) {
 
−
  bar;
 
−
  blam;
 
−
}
 
−
else {
 
−
  baz;
 
−
  zap;
 
−
}
 
−
</pre>
 
  
−
Braces for single statements are optional (and discouraged, but some still include them).
+
Some committers will want to re-create your changed nib before checkin, so make sure you indicate changes if they aren't obvious.
  
−
=== Proper patch format ===
+
== Requesting Review ==
  
−
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.
+
===Choosing a Reviewer===
 +
When requesting review, always request an initial review from one of the reviewers listed [[#Reviewers and Owners|below]] and then, '''*after*''' (and only after) receiving '''review+''' from two of them, request super-review from one of the super-reviewers.
  
−
To make a patch including a new file, first add <pre>/Foo.h/0/dummy timestamp//</pre> to <tt>path/to/file/CVS/Entries</tt> then diff as normal.
+
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 [[#Reviewers and Owners|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 [http://hg.mozilla.org/camino/log/tip/src/application/MainController.mm hg log] for the file(s) you are hacking and [http://hg.mozilla.org/camino/annotate/tip/src/application/MainController.mm#l130 hg annotate] for the lines of code you are changing to see which reviewers have hacked or reviewed that code before (remember that checkins prior to mid-2010 are identified with the committer’s email rather than the patch author’s, so check the changeset’s checkin comment for the actual author and reviewers).
  
−
=== <tt>Localizable.strings</tt> ===
+
Also check the queue ([https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=Camino&type=review&requestee=&component=&group=type reviews], [https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=Camino&type=superreview&requestee=&component=&group=type super-reviews]) to see which reviewers are "backed up" before requesting review or super-review; you might also ask on [irc://irc.mozilla.org#camino 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 [irc://irc.mozilla.org#camino #camino] on IRC.
  
−
Any UI string that appears in the code rather than in a nib needs to have an entry in <tt>Localizable.strings</tt> (or, in rare instances, a preference pane's <tt>Localizable.strings</tt> or one of the other <tt>.strings</tt> files).
+
=== Requesting Review on a Patch ===
 +
To choose a reviewer, while on the upload attachment/patch page in Bugzilla, click on the <code><select></code> 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 [[#Reviewers and Owners|below]].  
  
−
If you make additions or changes to the <tt>Localizable.strings</tt> file, make a '''clear note''' in your comment when attaching your patch, indicating which strings should be added or changed.  '''Do not attempt to <tt>diff</tt> the file''' (<tt>.strings</tt> files are UTF-16, which <tt>diff</tt> does not understand properly), and '''do not attach changed <tt>Localizable.strings</tt> files''' (which tend to become stale and cause regressions).
+
(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.)
  
−
Use "curly quotes" in the actual strings.
+
== 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.
  
−
=== Project (<tt>Camino.xcode</tt>) changes ===
+
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 <tt>bugnumber.patch</tt> becomes <tt>bugnumber_v1.1.patch</tt> or <tt>bugnumber_v2.patch</tt> (depending on the magnitude of the changes) and the patch description in Bugzilla becomes "Fix 1.1" or similar.
−
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.
+
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 <tt>review</tt> flag on the new patch to '''+''' yourself, with a comment like “carrying over r+ from $reviewer”. Also -->set any other flags (a second '''review?''')<!-- to their values from the previous version of the patch-->, 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.)
  
−
We expect to update to an Xcode 2.1 project file in the near future.
+
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|Checking In]] section below.
  
−
=== Nib changes ===
+
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).
  
−
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 reviewers 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.
+
== 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).
  
−
When editing nibs, be sure to follow the guidelines in [[Development:Editing Nibs]].
+
''Bugzilla will match review requests on partial name or email addresses (but, unlike for the CC and assignee fields, does not have an autocomplete widget.  See the [[#IRC nick to Bugzilla “unique matches” for Reviewers|table]] at the end of this section for “unique matches” you can use in the review request field to avoid both dropped review requests due to non-matches or duplicate matches and also having to look up reviewer emails.''
  
−
== Reviewers and Owners ==
+
* Annoyance Blocking:
−
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''': ardissone, <!--smfr, -->smorgan
 +
** '''Pop-up blocking''': murph, smorgan
 +
* Bookmarks: smorgan, cl
 +
* '''Build Config''': mento, ardissone, smorgan
 +
* '''Cocoa UI''': <!--BruceD, Wevah, -->murph
 +
* Downloading: kreeger, smorgan, ilya
 +
* '''Gecko-related changes/CH-embedding''': <!--hwaara,--> kreeger, smorgan
 +
* History: <!--smfr, -->smorgan, hendy
 +
* 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, hendy
 +
* '''Nib changes''': ardissone (''primarily layout/style and access conformance''), <!--froodian, -->smorgan
 +
* OS Integration:
 +
** '''AppleScript''': <!--smfr, peeja-->ardissone (sdef changes)
 +
** '''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, -->smorgan
 +
*: ''(though these bugs are most likely Core bugs that should be kicked elsewhere)''
 +
* Product Site: ss<!--, Wevah-->, ardissone<!--, isaac-->
 +
* Security: <!--smfr, -->smorgan, murph (safebrowsing)
 +
* '''Strings changes''': ardissone
 +
* Tabbed Browsing: smorgan, jeff, <!--froodian, -->murph
 +
* Toolbars & Menus: <!--froodian, -->cl
 +
* Translations: marcello
  
−
* '''Ad-blocking''': smokey, smfr
+
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). 
−
* Bookmarks: smorgan
 
−
* '''Build Config''': mento
 
−
* '''Cocoa UI''': BruceD, Wevah
 
−
* Downloading: kreeger
 
−
* '''Gecko-related changes/CH-embedding''': hwaara, kreeger, smorgan
 
−
* History: smfr, smorgan
 
−
* HTML Form Controls: smfr, smokey (forms.css changes), cflawson (forms.css changes)
 
−
*: forms.css changes require a [http://www.mozilla.org/hacking/reviewers.html Mozilla sr] (smfr); "controversial" ones require approval from dbaron
 
−
* '''Input Methods (IME)''': smfr
 
−
* Location Bar & Autocomplete:
 
−
* '''Nib changes''': smokey (primarily layout/style and access conformance), froodian
 
−
* Page Layout:
 
−
* Plugins: smfr
 
−
* '''RSS/feeds''': kreeger
 
−
* 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''' jeff, jpellico, aschulm or delliott.
 +
-->
 +
'''Exceptions:'''
  
−
In addition to the reviewers listed above, initial reviews can be requested from jpellico. smokey does not review code changes, but will test patches and make Xcode 1.5 project changes.
+
* 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 significant Product Site changes.
 +
* Check with Nick Kreeger [kreeger] before requesting review from him.
 +
* 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. -->
 +
<!--* Check with Josh Aas [josh] before requesting review or super-review from him.-->
 +
* 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.
  
−
Website changes can be reviewed by ss, maxr, or Wevah.
+
===IRC nick to Bugzilla “unique matches” for Reviewers===
 +
<!-- 'sortable' isn't working, so hide all the "no" and sort manually :/ -->
 +
{| class="wikitable sortable" style="width: 100%"
 +
|-
 +
! style="width: 25%" | Name !! style="width: 25%" | IRC nick !! style="width: 25%" | “unique match” !! style="width: 25%" | Currently Reviewing?
 +
|-
 +
| Mike Pinkerton || pinkerton || mikepink || Super-reviews only
 +
|-
 +
| Stuart Morgan || smorgan || stuart.morgan || Yes
 +
|-
 +
| Chris Peterson || cpeterso || @cpeterso. || Yes
 +
|-
 +
| Ilya Sherman || ilya || ishermandom || Yes
 +
|-
 +
| Smokey Ardisson || ardissone, sauron || alqahira || Yes, per “Exceptions”
 +
|-
 +
| Samuel Sidler || ss || :ss || Yes, per “Exceptions”
 +
|-
 +
| Marcello Testi || Pinolo || m.testi || Feedback on l10n issues only
 +
|-
 +
| Jeff Dlouhy || jeff || jeff.dlouhy  || Ask
 +
|-
 +
| Christopher Henderson || hendy || trendyhendy || Ask
 +
|-
 +
| Nick Kreeger || kreeger || nick.kreeger || Ask
 +
|-
 +
| Chris Lawson || cl || cl-bugs || Ask
 +
|-
 +
| Sean Murphy || murph || seanmurph || Ask
 +
|-
 +
| Håkan Waara || hwaara || hwaara || Ask
 +
|-
 +
| Nate Weaver || Wevah || Wevah || Ask
 +
|-
 +
| Mark Mentovai || mento || mark@moxie || Dire emergencies
 +
<!--
 +
|-
 +
| Simon Fraser || smfr || @smfr || No
 +
|-
 +
| Peter Jaros || peeja || :peeja || No
 +
|-
 +
| Ian Leue || froodian || froodian || No
 +
|-
 +
| Desmond Elliott || delliott || des.elliott || No
 +
|-
 +
| Aaron Schulman || aschulm || aschulm || No
 +
|-
 +
| Julian Pellico || julian || jpellico@yahoo.com || No
 +
|-
 +
| Dan Weber || dan || dan.j.weber || No
 +
-->
 +
|}
  
 
== “Super” Reviews ==
 
== “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 or super-reviewing Camino patches.)
+
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 do not typically require super-review; reviewers should test the nib thoroughly to make sure it satisfies all the conditions in the [[#Nib changes|Nib changes]] section above.  Nib changes taking place without an associated code change should get approval from a super-reviewer, however.
+
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|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 ==
 
== Checking In ==
−
After a patch has <tt>review+</tt> from two reviewers and <tt>superreview+</tt> 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 froodian, hwaara and kreeger.
+
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, mento, pinkerton, 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 your patch in after it has received the requisite approvals, add <tt>checkin-needed</tt> 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 (and the tree isn’t closed), make a comment in the bug or on [irc://irc.mozilla.org#camino IRC].
 +
 
 +
If you’re a new committer, information about checking in code to Camino repositories can be found on [[Development:Committing]].
 +
 
 +
==Helpful Tips for New Reviewers==
 +
 
 +
Congratulations, you’ve received your first review request!  Here are some handy tips to help you in your quest to keep bugs out of Camino and to improve the quality of Camino code.
 +
 
 +
===Review Timeframe===
 +
 
 +
In general, please try to review a patch within one week of receiving the review request.  Keeping the review queue moving helps keep development moving, unblocks other developers, and helps prevent contributors from being discouraged (“why should I spend my valuable time fixing this if it will just wait forever for a review?”).
 +
 
 +
If you know you will be unable to review a patch within the recommended timeframe, ask around to see if there is another reviewer who might be able to tackle the review before you can get to it (and, if so, redirect the review request to that person yourself, which will cause Bugzilla to send the ultimate reviewer’s review bugmail to the patch author).  If you can’t find another reviewer to help you out, or if you later experience unforeseen delays, it is polite to leave a note in the bug that it will take you longer (than expected) to get to the review.
 +
 
 +
===Viewing and Commenting on a Patch===
 +
 
 +
The Bugzilla patch viewing user interface is pretty awful and also quite buggy.  Here are a few tips to help you avoid common problems.
 +
 
 +
In the attachments table on the bug, two links are present for each patch, “Details” and “Diff” (and the patch description links to the raw patch file itself).  Each of these views is useful for reviewing, although the sheer number of bugs related to the “Diff” view negate most of its usefulness.  As a result, most of the review process takes place using the “Details” view (but you should feel free to open the “Diff” view in another tab if it helps you visualize the changes).
 +
 
 +
====Diff view====
 +
The “Diff” view ''attempts'' to present a pretty, visual diff view of the patch (by default against “the current source code,” though there are also mostly-broken “inter-diff” functions that allow comparing one patch against an alternate patch, selectable via the <select> control).  Most of the “Diff” view’s features only worked partially with CVS diffs and fall apart almost totally with Hg diffs.  In particular, any file moves/renames or deletions will be missing from the “Diff” view’s presentation of a patch.
 +
 
 +
Worse, the “Diff” view suffers from a bug that is particularly devastating when working with Objective-C (and certain other file formats, such as jar manifests).  {{bug|233695}} was filed years ago and will likely never be fixed.  It unfortunately causes any lines that begin with '''+''' or '''-''' characters to disappear, making many Obj-C function vanish into the ether.  (See https://bugzilla.mozilla.org/attachment.cgi?id=478667&action=diff#a/src/application/MainController.mm_sec2 and https://bugzilla.mozilla.org/attachment.cgi?id=478667&action=diff#a/src/application/MainController.mm_sec3 for an example; it appears, despite the <code> [self setupStartpage];</code> in the existing code for the third hunk, that there was never a <code>setupStartpage</code> function in the original hunk 2.)
 +
 
 +
If you know that a patch doesn’t touch any functions like this, it’s safe to use the “Diff” view for its comparatively better view of the changes.  Otherwise, though, you should avoid the “Diff” view (and the “Raw Unified” view, which suffers from the same bug).
 +
 
 +
====Details view====
 +
The “Details” view is thus the main view for interacting with patches.  This view loads the raw patch file into an <iframe> at the top of the page, with associated buttons below it, followed by the familiar comments box, and finally the review request <select>s.  If you wish to make comments on specific sections of code, the easiest way to do that is to click the “Edit Attachment As Comment” button, which will transform the patch file-in-iframe into an editable copy of the patch in a text field. Cut, paste, and comment at will. 
 +
 
 +
Be sure to ask questions where things are unclear and to suggest alternatives or different code patterns where warranted (see also [[#Things to Look For|Things to Look For]] below).  Hopefully by the time you receive your first review request, you will have received several good reviews from existing Camino reviewers and will have a good idea of the things to ask, suggest, and critique.
 +
 
 +
====Review outcomes====
 +
After leaving comments, there are three possible review outcomes: r-, r+ with comments (or nits), and r+.  Be sure to set the appropriate flag value after finishing your comments but before clicking the “Submit” button.
 +
 
 +
In an r- case, there are usually many or substantial issues with the patch, or there are minor problems but the solution involves enough code churn that you want to re-review the patch.  If you’ve been making comments as you go along, you may wish to end with a summary of the points, e.g. “r- for foo, lifetime issues with bar, and baz.”
 +
 
 +
In an “r+ with comments” case, there are some minor code or stylistic issues with the patch, but you do not feel that you need to re-review the patch after the fixes have been made.  (In some of these cases where the remaining issues are trivial or nits, you may even request super-review on the patch for the author when you finish your review.)  Again, you may conclude your review with “r=ircnick with foo and bar fixed.“
 +
 
 +
The rarest case is the straight-up r+: you’ve found nothing that needs changing in the patch (you may still have comments, perhaps recommending a follow-up bug be filed on something only tangentially related to the patch).  These reviews typically conclude with “r=ircnick” and request super-review on the patch (unless you or the patch author have requested a second review).
 +
 
 +
In cases where you are unsure about code, the proper behavior, or the like, ask other developers on irc.  On particularly complicated patches or patches covering code with which you’re very unfamiliar, you may wish to request a more experienced developer also review the patch after your review; set the “addl. '''review'''” flag and target that developer before submitting your review comments.
 +
 
 +
If you are ever asked to review a patch you can’t easily test, you can set the “addl. '''review'''” (or “feedback”, as appropriate) flag and target another developer or community member.
  
−
Also note that the <tt>mozilla/camino</tt> 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.).
+
It is helpful to prefix any minor comments with "nit: ", as in "nit: please use spaces rather than tabs here".  This helps the contributor give proper focus to the more important review comments, and makes reviews with lots of small comments feel less overwhelming.
  
−
====A tip for checking in nibs====
+
===Things to Look For===
−
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.
 
  
−
====Trunk/branch mismatches====
+
* Camino [[Development:Coding#Code_Style|code style]] (and also [[Talk:Development:Reviewing]], relevant parts of which should be moved into Coding)
−
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).
+
* […]
 +
* Things you remember being pointed out to you in your patches
  
−
# Set up your cvs environment as normal (i.e., <code>export cvsroot</code>)
+
In addition, unless explicitly mentioned otherwise, you are expected to build with the patch and test against various possible scenarios.
−
# <code>cvs co -r MOZILLA_1_8_BRANCH -d myfolder mozilla/camino/src/browser/foo mozilla/camino/PreferencePanes/Naviagtion/bar</code>
 
−
#: where <code>MOZILLA_1_8_BRANCH</code> is the branch tag, <code>myfolder</code> is the folder in which you want the stub tree to live, and <code>foo</code> and <code>bar</code> are the files contained in the patch
 
−
# Apply the patch and commit the files "as normal"
 

Latest revision as of 13:35, 2 February 2011

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. Do not include nibs or other binary resource files as part of a git-style Hg patch.

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 hg log for the file(s) you are hacking and hg annotate for the lines of code you are changing to see which reviewers have hacked or reviewed that code before (remember that checkins prior to mid-2010 are identified with the committer’s email rather than the patch author’s, so check the changeset’s checkin comment for the actual author and reviewers).

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

Bugzilla will match review requests on partial name or email addresses (but, unlike for the CC and assignee fields, does not have an autocomplete widget. See the table at the end of this section for “unique matches” you can use in the review request field to avoid both dropped review requests due to non-matches or duplicate matches and also having to look up reviewer emails.

  • 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: kreeger, smorgan
  • History: smorgan, hendy
  • 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: ardissone (sdef changes)
    • 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, murph (safebrowsing)
  • 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 significant Product Site changes.
  • Check with Nick Kreeger [kreeger] before requesting review from him.
  • Check with Håkan Waara [hwaara] before requesting review from him.
  • 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.

IRC nick to Bugzilla “unique matches” for Reviewers

Name IRC nick “unique match” Currently Reviewing?
Mike Pinkerton pinkerton mikepink Super-reviews only
Stuart Morgan smorgan stuart.morgan Yes
Chris Peterson cpeterso @cpeterso. Yes
Ilya Sherman ilya ishermandom Yes
Smokey Ardisson ardissone, sauron alqahira Yes, per “Exceptions”
Samuel Sidler ss :ss Yes, per “Exceptions”
Marcello Testi Pinolo m.testi Feedback on l10n issues only
Jeff Dlouhy jeff jeff.dlouhy Ask
Christopher Henderson hendy trendyhendy Ask
Nick Kreeger kreeger nick.kreeger Ask
Chris Lawson cl cl-bugs Ask
Sean Murphy murph seanmurph Ask
Håkan Waara hwaara hwaara Ask
Nate Weaver Wevah Wevah Ask
Mark Mentovai mento mark@moxie Dire emergencies

“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, mento, pinkerton, 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 your patch in 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 (and the tree isn’t closed), make a comment in the bug or on IRC.

If you’re a new committer, information about checking in code to Camino repositories can be found on Development:Committing.

Helpful Tips for New Reviewers

Congratulations, you’ve received your first review request! Here are some handy tips to help you in your quest to keep bugs out of Camino and to improve the quality of Camino code.

Review Timeframe

In general, please try to review a patch within one week of receiving the review request. Keeping the review queue moving helps keep development moving, unblocks other developers, and helps prevent contributors from being discouraged (“why should I spend my valuable time fixing this if it will just wait forever for a review?”).

If you know you will be unable to review a patch within the recommended timeframe, ask around to see if there is another reviewer who might be able to tackle the review before you can get to it (and, if so, redirect the review request to that person yourself, which will cause Bugzilla to send the ultimate reviewer’s review bugmail to the patch author). If you can’t find another reviewer to help you out, or if you later experience unforeseen delays, it is polite to leave a note in the bug that it will take you longer (than expected) to get to the review.

Viewing and Commenting on a Patch

The Bugzilla patch viewing user interface is pretty awful and also quite buggy. Here are a few tips to help you avoid common problems.

In the attachments table on the bug, two links are present for each patch, “Details” and “Diff” (and the patch description links to the raw patch file itself). Each of these views is useful for reviewing, although the sheer number of bugs related to the “Diff” view negate most of its usefulness. As a result, most of the review process takes place using the “Details” view (but you should feel free to open the “Diff” view in another tab if it helps you visualize the changes).

Diff view

The “Diff” view attempts to present a pretty, visual diff view of the patch (by default against “the current source code,” though there are also mostly-broken “inter-diff” functions that allow comparing one patch against an alternate patch, selectable via the <select> control). Most of the “Diff” view’s features only worked partially with CVS diffs and fall apart almost totally with Hg diffs. In particular, any file moves/renames or deletions will be missing from the “Diff” view’s presentation of a patch.

Worse, the “Diff” view suffers from a bug that is particularly devastating when working with Objective-C (and certain other file formats, such as jar manifests). Bug 233695 was filed years ago and will likely never be fixed. It unfortunately causes any lines that begin with + or - characters to disappear, making many Obj-C function vanish into the ether. (See https://bugzilla.mozilla.org/attachment.cgi?id=478667&action=diff#a/src/application/MainController.mm_sec2 and https://bugzilla.mozilla.org/attachment.cgi?id=478667&action=diff#a/src/application/MainController.mm_sec3 for an example; it appears, despite the [self setupStartpage]; in the existing code for the third hunk, that there was never a setupStartpage function in the original hunk 2.)

If you know that a patch doesn’t touch any functions like this, it’s safe to use the “Diff” view for its comparatively better view of the changes. Otherwise, though, you should avoid the “Diff” view (and the “Raw Unified” view, which suffers from the same bug).

Details view

The “Details” view is thus the main view for interacting with patches. This view loads the raw patch file into an <iframe> at the top of the page, with associated buttons below it, followed by the familiar comments box, and finally the review request <select>s. If you wish to make comments on specific sections of code, the easiest way to do that is to click the “Edit Attachment As Comment” button, which will transform the patch file-in-iframe into an editable copy of the patch in a text field. Cut, paste, and comment at will.

Be sure to ask questions where things are unclear and to suggest alternatives or different code patterns where warranted (see also Things to Look For below). Hopefully by the time you receive your first review request, you will have received several good reviews from existing Camino reviewers and will have a good idea of the things to ask, suggest, and critique.

Review outcomes

After leaving comments, there are three possible review outcomes: r-, r+ with comments (or nits), and r+. Be sure to set the appropriate flag value after finishing your comments but before clicking the “Submit” button.

In an r- case, there are usually many or substantial issues with the patch, or there are minor problems but the solution involves enough code churn that you want to re-review the patch. If you’ve been making comments as you go along, you may wish to end with a summary of the points, e.g. “r- for foo, lifetime issues with bar, and baz.”

In an “r+ with comments” case, there are some minor code or stylistic issues with the patch, but you do not feel that you need to re-review the patch after the fixes have been made. (In some of these cases where the remaining issues are trivial or nits, you may even request super-review on the patch for the author when you finish your review.) Again, you may conclude your review with “r=ircnick with foo and bar fixed.“

The rarest case is the straight-up r+: you’ve found nothing that needs changing in the patch (you may still have comments, perhaps recommending a follow-up bug be filed on something only tangentially related to the patch). These reviews typically conclude with “r=ircnick” and request super-review on the patch (unless you or the patch author have requested a second review).

In cases where you are unsure about code, the proper behavior, or the like, ask other developers on irc. On particularly complicated patches or patches covering code with which you’re very unfamiliar, you may wish to request a more experienced developer also review the patch after your review; set the “addl. review” flag and target that developer before submitting your review comments.

If you are ever asked to review a patch you can’t easily test, you can set the “addl. review” (or “feedback”, as appropriate) flag and target another developer or community member.

It is helpful to prefix any minor comments with "nit: ", as in "nit: please use spaces rather than tabs here". This helps the contributor give proper focus to the more important review comments, and makes reviews with lots of small comments feel less overwhelming.

Things to Look For

In addition, unless explicitly mentioned otherwise, you are expected to build with the patch and test against various possible scenarios.

Retrieved from "http://wiki.caminobrowser.org/index.php?title=Development:Reviewing&oldid=11671"