Development:Reviewing - Revision history

Sardisson: /* IRC nick to Bugzilla “unique matches” for Reviewers */ move cl to ask and add cpeterso now that he's been around

2011-02-02T21:35:18Z

IRC nick to Bugzilla “unique matches” for Reviewers: move cl to ask and add cpeterso now that he's been around

Sardisson: /* Details view */ s/developers/reviewers/

2010-12-12T21:13:50Z

Details view: s/developers/reviewers/

Sardisson: /* IRC nick to Bugzilla “unique matches” for Reviewers */ move hendy to "ask" since we haven't heard back from him

2010-12-09T18:13:48Z

IRC nick to Bugzilla “unique matches” for Reviewers: move hendy to "ask" since we haven't heard back from him

Sardisson: /* Reviewers and Owners */ both…also

2010-10-28T20:41:08Z

Reviewers and Owners: both…also

Sardisson: /* Reviewers and Owners */ clarify

2010-10-28T20:40:04Z

Reviewers and Owners: clarify

Sardisson: /* Reviewers and Owners */ add table, hide Mano entirely

2010-10-28T20:29:32Z

Reviewers and Owners: add table, hide Mano entirely

Isherman: /* Review outcomes */

2010-10-17T00:03:39Z

Review outcomes

Sardisson: /* Reviewers and Owners */ add murph/safebrowsing to security

2010-10-15T18:36:52Z

Reviewers and Owners: add murph/safebrowsing to security

Sardisson: /* Reviewers and Owners */ add hendy to history, optimistically (at least until hiding him everywhere)

2010-10-15T18:35:53Z

Reviewers and Owners: add hendy to history, optimistically (at least until hiding him everywhere)

Sardisson: /* Choosing a Reviewer */ update this for Hg (oops!)

2010-10-15T18:34:05Z

Choosing a Reviewer: update this for Hg (oops!)

@@ Line 115: / Line 115: @@
 | Stuart Morgan || smorgan || stuart.morgan || Yes
 |-
-| Chris Lawson || cl || cl-bugs || Yes
+| Chris Peterson || cpeterso || @cpeterso. || Yes
 |-
 | Ilya Sherman || ilya || ishermandom || Yes
@@ Line 130: / Line 130: @@
 |-
 | Nick Kreeger || kreeger || nick.kreeger || Ask
 |-
 | Sean Murphy || murph || seanmurph || Ask

@@ Line 196: / Line 196: @@
 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 developers and will have a good idea of the things to ask, suggest, and critique.
+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====

@@ Line 114: / Line 114: @@
 |-
 | Stuart Morgan || smorgan || stuart.morgan || Yes
 |-
 | Chris Lawson || cl || cl-bugs || Yes
@@ Line 128: / Line 126: @@
 |-
 | Jeff Dlouhy || jeff || jeff.dlouhy  || Ask
 |-
 | Nick Kreeger || kreeger || nick.kreeger || Ask

@@ Line 57: / Line 57: @@
 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 [[#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 having to look up reviewer emails.''
+''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.''
 * Annoyance Blocking:

@@ Line 57: / Line 57: @@
 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 complete 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 and having to look up reviewer emails.''
+''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 having to look up reviewer emails.''
 * Annoyance Blocking:

@@ Line 56: / Line 56: @@
 == 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:
@@ Line 89: / Line 91: @@
 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).
-<!--'''In addition to the reviewers listed above, initial reviews can be requested from''' jeff, jpellico, aschulm or delliott.-->
+<!--'''In addition to the reviewers listed above, initial reviews can be requested from''' jeff, jpellico, aschulm or delliott.
 -->
 '''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.
+* 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.
+<!--* 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.
 == “Super” Reviews ==

← Older revision		Revision as of 00:03, 17 October 2010
Line 157:		Line 157:

	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.		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===		===Things to Look For===

@@ Line 81: / Line 81: @@
 *: ''(though these bugs are most likely Core bugs that should be kicked elsewhere)''
 * Product Site: ss<!--, Wevah-->, ardissone<!--, isaac-->
-* Security: <!--smfr, -->smorgan
+* Security: <!--smfr, -->smorgan, murph (safebrowsing)
 * '''Strings changes''': ardissone
 * Tabbed Browsing: smorgan, jeff, <!--froodian, -->murph

@@ Line 65: / Line 65: @@
 * Downloading: kreeger, smorgan, ilya
 * '''Gecko-related changes/CH-embedding''': <!--hwaara,--> kreeger, smorgan
-* History: <!--smfr, -->smorgan
+* History: <!--smfr, -->smorgan, hendy
 * HTML Form Controls:
 *: ''(these bugs are likely Core bugs that should be kicked to Widget:Cocoa in most cases)''

@@ Line 34: / Line 34: @@
 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.
-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://bonsai.mozilla.org/cvslog.cgi?file=mozilla/camino/src/application/MainController.mm&rev=HEAD&mark=HEAD bonsai's cvs log] for the file(s) you are hacking and [http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/camino/src/application/MainController.mm&rev=HEAD&mark=137-150#130 bonsai's cvs blame] for the lines of code you are changing to see which reviewers have hacked or reviewed that code before.
+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).
 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.