Personal tools

Firefox/Code Review

From MozillaWiki

< Firefox
Revision as of 15:00, 25 June 2013 by GavinSharp (Talk | contribs)

(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

Contents

Reviewers

Firefox reviewers are the people who make sure that Firefox is awesome, and that patches do no harm. As in other modules, reviewers in Firefox might not have expertise in all aspects of the Firefox code, but any Firefox reviewer can review patches in any part of Firefox if they feel that they are able to do so appropriately. (For purposes of Mozilla processes, such as vouching for commit access, all Firefox reviewers are considered peers.)

Reviewers in the Firefox module are expected to:

  • know their limits, and decline or (preferably) redirect to those more qualified as needed;
  • have interest, if not expertise, in all aspects of Firefox;
  • speak their minds clearly but considerately;
  • support the decisions of their peers, though they need not censor their own polite disagreement;
  • participate in the development of Firefox contributors;
  • represent Firefox architecture and implementation issues to other groups or projects; and
  • look for opportunities to improve Firefox as software, a product, and a community.

Any person with level 3 commit access can become a Firefox reviewer given the recommendation of 3 other reviewers.

What does r+ mean?

Or, "what to expect when you're granting a review".

Review being granted in Firefox means:

  • I don't believe that this patch does harm. Areas of focus include:
    • security concerns, especially where content and chrome touch each other;
    • significant performance issues, especially on key paths like startup and page load;
    • localization and accessibility issues.
  • If it does cause problems, I will help make it right.
  • The patch has test coverage appropriate to the change.
  • Saying Thank you for your patch!

Review in Firefox is not conditional on:

  • The patch being the best possible way that the reviewer can envision of achieving its aim.
  • The patched code reads as if it were written by a single person. Style should be consistent, but it need not be identical.

Making a good patch

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 help you, and don't be surprised if you get asked about one of the below. (Don't be ashamed either, this stuff is complicated and everyone forgets now and then.)

  • small patches, functionally divided;
  • description of any security concerns, especially where content and chrome touch each other;
  • significant performance issues, especially on key paths like startup and page load;
  • localization and accessibility issues;
  • some tests;
  • descriptive commit messages including your name for attribution.

Help, my patch isn't being reviewed

The reviewer list might look long, but these are busy people and there can be a lot of patches to review. Sometimes patches fall through the cracks, for which we are all quite sorry.

A poke in the bug can help, or direct email to the designated reviewer. If that doesn't work, please mail gavin and/or dolske for assistance.

Are you looking for UI review or feedback but not getting it? Make sure to flag ux-review@mozilla.com for your UI-review/feedback request and it should be answered within 48 hours.