Effective Code Review
(Originally based on User:GavinSharp/Code_Review)
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! :-)
- 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
- 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. :-)