Code reviews

This checklist is primarily aimed at reviewers, as it lists important points to check while reviewing a patch.

It can also be useful for patch authors: if the changes comply with these guidelines, then it's more likely the review will be approved.

Bug status and patch file

Bug status is assigned, and assignee is correctly set.
Commit title and message follow the conventions.
Commit message says what is being changed and why.
Patch applies locally to current sources with no merge required.
Check that every new file introduced by the patch has the proper Mozilla license header: https://www.mozilla.org/en-US/MPL/headers/

Verify:
- if it's a new feature, the patch implements it.
- if it's a fix, the patch fixes the bug it addresses.
Report any problems you find in the global review comment.
Decide if any of those problems should block landing the change, or if they can be filed as follow-up bugs instead, to be fixed later.

Run new/modified tests, with and without e10s.
Watch out for tests that pass but log exceptions or end before protocol requests are handled.
Watch out for slow/long tests: suggest many small tests rather than single long tests.
Watch out for new tests written as integration tests instead of as unit tests: unit tests should be the preferred option, when possible.

Code changes:
Review only what was changed by the contributor.
Code formatting follows our ESLint rules and coding standards.
Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see the comment guidelines.
If Promise code was added/modified, the right promise syntax is used and rejections are handled. See asynchronous code.
If a CSS file is added/modified, it follows the CSS guidelines.
If a React or Redux module is added/modified, it follows the React/Redux guidelines.
If DevTools server code that should run in a worker is added/modified then it shouldn't use Services
Test changes:
The feature or bug is tested by new tests, or a modification of existing tests.
Test logging is sufficient to help investigating test failures/timeouts.
Test is e10s compliant (no CPOWs, no content, etc…).
Tests are clean and maintainable.
A try push has started (or even better, is green already).
User facing changes:
If a new piece of UI or new user interaction is added/modified, then UX is ui-review? on the bug.
If a user facing string has been added, it is localized and follows the localization guidelines.
If a user-facing string has changed meaning, the key has been updated.
If a new image is added, it is a SVG image or there is a reason for not using a SVG.
If a SVG is added/modified, it follows the SVG guidelines.
If a documented feature has been modified, the keyword dev-doc-needed is present on the bug.

R+: the code should land as soon as possible.
R+ with comments: there are some comments, but they are minor enough, or don't require a new review once addressed, trust the author.
R cancel / R- / F+: there is something wrong with the code, and a new review is required.