User:Ehsan/Code Review

From MozillaWiki
Jump to: navigation, search

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)


  • 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. :-)