Code Review

Reviewer Guidelines

1. Prioritize reviews highly. Reviewing code is more valuable than writing code as it results in higher overall project activity. If you find you can't write code any more due to prioritizing reviews over coding, appoint more reviewers.
2. Communicate. If you are an active contributor, you should respond to a review request within one working day of getting it, either with a review, a deadline by which you promise to do the review, or a polite refusal. If you think a patch is lower priority than your other work communicate that.
3. Silence is not an option. If you think saying nothing is better than admitting than you won't get to the patch for a while, that's being passive-aggressive. This is not a good way to build a happy coding community. Holding back bad code is a feature, not a bug - so do it, quickly and politely.

Patch Author Guidelines

There are some things that make it easier to get a good, prompt review, and to improve the quality of the patch you submit. Help your reviewers review your patches quickly by making patches easy to review:

• Split out mass-changes or mechanical changes into a separate patch from the substantive changes.
• Separate patches into conceptually-separate pieces for review purposes (even if you then later collapse them into a single changeset to push), if technically possible. Any time you're requesting review from multiple people on a single huge diff, chance are splitting it might have been a good idea.
• Make the summary of the bug reflect the problem rather than some particular symptom of it, so there's a clear description of what the patch is trying to fix
• Write good commit messages that describe what's changing between old and new code (which, if it can't be summarized in less than about 100-150 characters, should have a short summary on the first line and a longer description on later lines)
• Write good code comments that describe the state of the new code, and if the patch is of nontrivial size, point to the important comments in the non-first lines of the commit message
• Address all comments from previous reviews (either by fixing as requested, or explaining why you haven't) before requesting another review. It's very common to just ignore (miss?) some of the review comments, which means the reviewer then needs to triple-check that all the things they pointed out got fixed.
• Provide an interdiff for second and subsequent patches when requesting review for them. This is so that the reviewer can verify that the changes you made match what they asked for. (Bugzilla's interdiff is totally unsuitable for this purpose, unfortunately, because it fails so often because it doesn't have access to the original code.)
• If your request only relates to part of the patch, say so clearly.
• Don't request review from people who are labelled as being away in Bugzilla.

Potential Bugzilla Improvements

• In addition to r+/r- add a Review ETA field where the requested reviewer can specify when the patch will get reviewed. Bugzilla should send reminder emails based on that flag.
  • Reminders should be set to every 24-hours by default
  • it should be possible to get reminders for multiple people (eg for managers to track review latency of whole team)
  • reminders should specify how long has it been since reviewer last commented in the bug
• Provide information about reviewers when asking for r?
  • Add a notion of being away to Bugzilla accounts, or integrate with a Time Off tracking app.
  • Display a user's number of outstanding reviews
• Add a 'low-priority' patch flag
• Some integration with a formatting checker

Tips and Tricks

• When responding to a review, rebase the patch first, then generate any responses to the review as a separate patch in mercurial. You can post that as the interdiff for the reviewer, and (when ready) hg qfold it into the original (rebased) patch.
• hg diff -r qparent (perhaps aliased in .hgrc to qdiffall) is an easy way to get a rollup of all changes currently applied in the patch queue (such as base patch(es) plus interdiffs.
• See https://firefox-source-docs.mozilla.org/contributing/Code_Review_FAQ.html for a checklist of what to look for while actually reviewing changes.