DevTools/Code Review Checklist

This checklist is primarily useful for DevTools reviewers as it lists all points potentially important to check while reviewing code, but it can serve as a set of guidelines for patch authors too.

Bug status and patch file

• Bug status is assigned, and assignee is correctly set
• Commit title and message follow the convention at : https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
• Commit message says what is being changed and why as described here : http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#write-detailed-commit-messages
• Patch applies locally to current sources with no merge required

Manual testing

• Verify the feature or fix
• Report problems in the global review comment
• Decide which bugs need to block landing the change, and which can be fixed as follow-ups instead

Automated testing

• 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

Code review

• Code changes
  • Review only what was changed by the contributor
  • Code formatting follows our ES Lint rules, see CodeStyle documentation at : https://wiki.mozilla.org/DevTools/CodingStandards
  • Code makes use of ES features when relevant
  • Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see Comment guidelines at : https://wiki.mozilla.org/DevTools/CodingStandards#Comments
  • If Promise code was added/modified, the right promise syntax is used, rejection are handled and Tasks are used when possible : https://wiki.mozilla.org/DevTools/CodingStandards#Asynchronous_Code
  • If a CSS file is added/modified it follows the guidelines at : https://wiki.mozilla.org/DevTools/CSSTips
  • If a React or Redux module is added/modified it follows the guidelines at : https://wiki.mozilla.org/DevTools/CodingStandards#React_.26_Redux
  • 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 : https://wiki.mozilla.org/DevTools/Hacking#Writing_Tests
  • Test logging is sufficient to help investigating test failures/timeouts : https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Logs_and_comments
  • Test is e10s compliant (no CPOWs, no content etc...). See : https://wiki.mozilla.org/DevTools/mochitests_coding_standards#E10S_.28Electrolysis.29
  • Tests follow the guidelines at : https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Writing_clean.2C_maintainable_test_code
  • 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 Helen is ui-review? on the bug
  • If a user facing string has been added, it is localized and follows the localization guidelines at : https://wiki.mozilla.org/DevTools/Hacking#Guidelines
  • If a user-facing string has changed meaning, the key has been updated : https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
  • 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 guidelines at : https://dxr.mozilla.org/mozilla-central/source/devtools/docs/svgs.md
  • If a documented feature has been modified, the keyword dev-doc-needed is present on the bug

Finalize the review

• 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