Effective Code Review

(Originally based on User:GavinSharp/Code_Review)

Preliminary questions

Things to ask yourself when you receive a review request.

• am I the best person to review this?
  • is someone else better suited?
  • is the patch suitable to hand out to someone to help them grow as a reviewer?
  • should other people at least be notified/involved?
  • is my current queue too big?
  • can be hard to not let pride get in the way!
• do I understand the bigger picture?
  • the reviewer may not have been previously involved with the bug
  • the first step is to understand the problem being solved
• should we do this?
  • sometimes this is very easy to answer, other times it's the hardest part
  • can sometimes rely on others in better positions to make that judgement (e.g. UX)
• is this the best way to do this?
  • easy to forget this step before getting into specifics
  • creativity and knowledge of common techniques/approaches is key - don't let the patch author's approach constrain your imagination
• is the patch too big, or doing too much at once?
  • don't be shy about asking for the patch to be split into multiple patches (or bugs)

Specifics

• have we addressed this problem before?
  • is there A Long History? (HG/CVS log/blame, archaeology, Netscape-era bugs!)
  • building relevant context is really important
• does the patch have tests?
  • what is the test coverage like? could it be more complete?
  • is the test in the right test suite?
• is the patch sufficient?
  • check that all manifestations of the bug are fixed - this is where domain knowledge is most useful
  • is the problem being addressed occurring in other code? has there been an audit?
  • is there future proof tests to ensure the problem never occurs again?
• is the patch complete?
  • obvious: refactorings - did you forget to update a caller? stuff like this should be the test's job, but we don't always have 100% coverage
  • code removal patches: check not only that the removals themselves are OK, but also that you removed everything
  • …but be careful not to scope creep!
• does the patch impact extensions/other products/gecko consumers?
  • modifying an XPIDL API
• does the patch expose something to the web?
  • modifying something in dom/webidl
  • exposing new CSS features
  • new WebGL features
  • other things a web page can invoke
• does the patch have performance implications?
  • hot paths (e.g. startup, pageload)
  • known bad patterns (e.g. sync reflows)
  • sync IO/IPC?
  • Locking overhead?
• does the patch have security implications?
  • raw pointer manipulations
  • refcounting balance correctness
  • missing overflow checks
  • trusting data coming from untrusted sources
• does the patch have memory consumption implications?
  • non-refcounted non-stack classes without MOZ_COUNT_CTOR/MOZ_COUNT_DTOR to opt into leak checking
  • JS leaks of references being held for too long through closures
  • watch out for the blind spots of our leak checking tests!
  • is the patch adding to the potential amount of the heap-unclassified memory?
• is the patch exposing an opportunity for detecting a bug at compile time? file a bug for a static analysis to be created for it as a regression prevention mechanism!
• be afraid of
  • threading!  :-)
• documentation
  • think of future us! think of what might be obvious now, but not later
  • add relevant references; keep in mind permanence of those references
  • write good commit messages; don't make people read all of those 110 bugzilla comments years later to understand what happened!
  • extra comments never hurt anyone (but don't write a novel!)
• platform specific concerns?
• grab bag of acronyms: l10n/rtl/a11y/1337
  • bring in domain experts when there's any doubt at all

Key Points

• don't be reluctant to redirect requests, or ask for additional help
• be friendly; particularly for first-time or infrequent contributors (doesn't always come naturally!)
• focus on substance, not style; outcomes, not process
• be open-minded; avoid the "NIH" mindset
• ask and encourage others to ask for feedback early. review shouldn't be about "big questions" - those should use a feedback request, or comments, or IRC, etc.
• please take the time to appreciate the work the author has done in the end. especially if you put them through a lot of (hopefully good!) comments. :-)