Firefox Media Playback Team Review Policy

Reviews form a central part of how we at Mozilla ensure engineering diligence. Prompt, yet thorough, reviews are also a critical component in maintaining team velocity and productivity. Reviews are also one of the primary ways that a distributed organization like Mozilla does its mentoring and development of team members.

So given how important reviews are, it pays to be deliberate about what you're aiming for.

The senior members of the Firefox Media Playback team met in Auckland in August 2016 to codify the roadmap, vision, and policy for the team, and and one of the things we agreed upon was our review policy.

The policy has served us well, as I think we've demonstrated with all we've achieved, so I'm sharing it here in the hope that it inspires others.

• Having fast reviews is a core value of the media team.
• Review should be complete by end of next business day.
• One patch for one logical scope or change. Don't cram everything into one patch!
• Do not fix a problem, fix the cause. Workarounds are typically bad. Look at the big picture and find the cause.
• We should strive for a review to be clear. In all cases it should be clear what the next course of action is.
• Reviews are there to keep bad code out of the tree.
• Bad code tends to bring out bad reviews.
• Commit message should describe what the commit does and why. It should describe the old bad behaviour, and the new good behaviour, and why the change needs to be made.
• R+ means I don’t want to see it again. Maybe with comments that must be addressed before landing.
• R- means I do want to see it again, with a list of things to fix.
• R canceled means we’re not going to review this.
• Anyone on the media team should be expected to complete a follow up bug.
• It’s not OK for a reviewer to ask a test to be split out from a changeset, provided the test is related to the commit. By the time a patch gets to review, splitting the test out doesn’t create value, just stop-energy.
• Review request. If response is slow, ping or email for a reminder, otherwise find another reviewer.
• Don’t be afraid to ask when the review will come. The reply to “when” can be “is it urgent?”
• Everyone should feel comfortable pointing out flaws/bugs as a “drive by”.
• Give people as much responsibility as they can handle.
• Reviewers should make it clear what they haven’t reviewed.
• American English spelling, for comments and code.
• Enforce Mozilla coding style, and encourage auto formatters, like `./mach clang-format`.
• Use reviewboard. Except when you can’t, like security patches.