Skip to content

Code Review

Jump to bottom
Martin Robinson edited this page Feb 9, 2024 · 1 revision

There are a few key things we look for when asking Servo contributors to become reviewers. Surprisingly, none of them are an understanding of the full breadth of code in Servo:

  • Get to reviews soon! You don’t have to complete the review, necessarily. Do a first pass, or if you really have no time then hand it off to someone else quickly.
  • Be courteous - you are a personal connection to Mozilla and you should be as welcoming as Mozilla is supposed to be.
  • Know your limits. Don’t review anything if you don’t understand the code it deals with. Instead, help the reviewee find an alternate reviewer.
  • You don’t get to demand that the reviewee re-write the patch in your preferred style. If their code meets style guidelines, fixes the bug, and is efficient enough, don’t waste people’s time by re-architecting it.

All of these elements can be observed through the code the potential reviewer is writing, and letting them review smaller patches by experienced contributors.

(Liberally adapted from Dave Townsend's blog post about Firefox reviewers)

All changes to Servo must be reviewed before they are checked in to the main Servo repository. For the list of reviewers, see contributing page at servo.org. We have a bot powered by Homu that helps enforce and automate the review workflow.

Recommended workflow for Servo code reviewers:

  1. Read the commit messages. Do they convey the intended changes appropriately? Make suggestions for ways to improve them, if necessary.

  2. Read the code changes. Consider starting with the pieces that are most relevant/complex, rather than reading top to bottom, in order to better understand why changes were made.

  3. If the changes are implementing a specification, ensure that the code changes follow the specification text. Leave comments on lines that are confusing or incorrect, possibly including suggestions for correcting them. Ensure that there are enough links back to the spec or mentions of steps in the spec.

  4. Is there appropriate documentation present for new data types, functions, methods, variants, etc.? Leave comments on lines that are missing useful documentation. Leave comments on lines that have documentation that is incomplete in some way - for example, using terms that are unclear in context, too high-level or low-level, etc.

  5. Consider if there are portions of the code changes that could be written in different ways that would improve clarity or reduce complexity. Leave comments with suggestions.

  6. Note any stylistic nits that should be addressed by leaving comments starting with nit:. Consider filing new issues for nits that could be reported via automated tools (example).

  7. Do the changes include updates to existing test expectations, indicating that the code in quested is already tested? Do they cover all of the code that is being changed? Leave comments describing the code that is not being tested and suggest how to do so.

  8. If the previous step is not satisfied, do the changes include at least one new test? Would further tests increase the confidence that the changes are correct in all cases? Leave comments suggesting specific desired test cases.

  9. Leave a summary comment to indicate that the review is complete. Remove the S-awaiting-review label and add an appropriate one (such as S-needs-code-changes or S-needs-tests) to indicate the next step for the PR.

  10. If the PR is ready to be tested and merged, write @bors-servo: r+ in your commit message. Homu will add the PR to its queue, and if tests run successfully then it will merge the PR.

Clone this wiki locally