docs/contributing/Code_Review_FAQ.rst

   1 Code Review FAQ
   2 ===============
   3
   4 What is the purpose of code review?
   5 -----------------------------------
   6
   7 Code review is our basic mechanism for validating the design and
   8 implementation of patches. It also helps us maintain a level of
   9 consistency in design and implementation practices across the many
  10 hackers and among the various modules of Mozilla.
  11
  12 Of course, code review doesn't happen instantaneously, and so there is
  13 some latency built into the system. We're always looking for ways to
  14 reduce the wait, while simultaneously allowing reviewers to do a good
  15 chunk of hacking themselves. We don't have a perfect system, and we
  16 never will. It's still evolving, so let us know if you have suggestions.
  17
  18 Mozilla used to have the concept of "super-review", but `a consensus was
  19 reached in
  20 2018 <https://groups.google.com/forum/#!topic/mozilla.governance/HHU0h-44NDo>`__
  21 to retire this process.
  22
  23 Who must review my code?
  24 ------------------------
  25
  26 You must have an approval ("r={{ mediawiki.external('name') }}") from
  27 the module owner or designated "peer" of the module where the code will
  28 be checked in. If your code affects several modules, then generally you
  29 should have an "r={{ mediawiki.external('name') }}" from the owner or
  30 designated peer of each affected module. We try to be reasonable here,
  31 so we don't have an absolute rule on when every module owner must
  32 approve. For example, tree-wide changes such as a change to a string
  33 class or a change to text that is displayed in many modules generally
  34 doesn't get reviewed by every module owner.
  35
  36 You may wish to ask others as well.
  37
  38
  39 What do reviewers look for?
  40 ---------------------------
  41
  42 A review is focused on a patch's design, implementation, usefulness in
  43 fixing a stated problem, and fit within its module. A reviewer should be
  44 someone with domain expertise in the problem area. A reviewer may also
  45 utilize other areas of his or her expertise and comment on other
  46 possible improvements. There are no inherent limitations on what
  47 comments a reviewer might make about improving the code.
  48
  49 Reviewers will probably look at the following areas of the code:
  50
  51 -  “goal” review: is the issue being fixed actually a bug? Does the
  52    patch fix the fundamental problem?
  53 -  API/design review. Because APIs define the interactions between
  54    modules, they need special care. Review is especially important to
  55    keep APIs balanced and targeted, and not too specific or
  56    overdesigned. There are a `WebIDL review
  57    checklist <https://wiki.mozilla.org/WebAPI/WebIDL_Review_Checklist>`__.
  58    There are also templates for emails that should be sent when APIs are
  59    going to be exposed to the Web and general guidance around naming on
  60    `this wiki
  61    page <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>`__.
  62 -  Maintainability review. Code which is unreadable is impossible to
  63    maintain. If the reviewer has to ask questions about the purpose of a
  64    piece of code, then it is probably not documented well enough. Does
  65    the code follow the :ref:`Coding style` ? Be careful when
  66    reviewing code using modern C++ features like auto.
  67 -  Security review. Does the design use security concepts such as input
  68    sanitizers, wrappers, and other techniques? Does this code need
  69    additional security testing such as fuzz-testing or static analysis?
  70 -  Integration review. Does this code work properly with other modules?
  71    Is it localized properly? Does it have server dependencies? Does it
  72    have user documentation?
  73 -  Testing review. Are there tests for correct function? Are there tests
  74    for error conditions and incorrect inputs which could happen during
  75    operation?
  76 -  Performance review. Has this code been profiled? Are you sure it's
  77    not negatively affecting performance of other code?
  78 -  License review. Does the code follow the `code licensing
  79    rules <http://www.mozilla.org/hacking/committer/committers-agreement.pdf>`__?
  80
  81
  82 How can I tell the status of reviews?
  83 -------------------------------------
  84
  85 When a patch has passed review you'll see "Accepted" in green at the top
  86 of a Phabricator revision, under the title. In Bugzilla (which is
  87 deprecated in favour of Phabricator), this is indicated by "{{
  88 mediawiki.external('name') }}:review+" in the attachment table in the
  89 bug report. If it has failed review then you'll see "Needs Revision" in
  90 red at the top of the revision, or, in Bugzilla, "{{
  91 mediawiki.external('name') }}:review-". Most of the time that a reviewer
  92 sets a review flag, they will also add a comment to the bug explaining
  93 the review.