Toolkit/Code Review

Who should review my patch?

For patches (or parts of patches) that affect code in the Toolkit directories any of the Toolkit reviewers can be asked to review your patch. Many of the reviewers have particular areas of expertise so it is often preferable to choose a reviewer based on the sub-module the patch changes.

• Patches that make behavioral or API changes to Toolkit should either be reviewed by the sub-module owner or at least get feedback from them in addition to review by a Toolkit reviewer.
• Significant UI changes should flag ux-review@mozilla.com for UI review, as well as review from an appropriate code reviewer. There is more information on UI reviews and feedback from the UX team on devmo.
• Super-review is required where required by the mozilla.org super-review policy.

Reviewers

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

Reviewers in the Toolkit module are expected to:

• respond to review requests in an appropriate timeframe;
• request additional reviews or super-reviews where they think necessary;
• know their limits, and decline or (preferably) redirect to those more qualified as needed;
• 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 Toolkit contributors;
• ensure that Toolkit code is appropriate for all Mozilla based applications, not just Firefox;
• represent Toolkit architecture and implementation issues to other groups or projects; and
• look for opportunities to improve the UI and APIs provided by Toolkit.

Any person with level 3 commit access can become a Toolkit 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 Toolkit 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 Toolkit 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.

Toolkit reviewers are free to ask other people in the community to review patches on their behalf where they think appropriate to help grow new reviewers. In this case it is essentially the Toolkit reviewer asserting all of the above so they may wish to do an quick additional review pass afterwards.

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