1Code Review FAQ
2===============
3
4What is the purpose of code review?
5-----------------------------------
6
7Code review is our basic mechanism for validating the design and
8implementation of patches. It also helps us maintain a level of
9consistency in design and implementation practices across the many
10hackers and among the various modules of Mozilla.
11
12Of course, code review doesn't happen instantaneously, and so there is
13some latency built into the system. We're always looking for ways to
14reduce the wait, while simultaneously allowing reviewers to do a good
15chunk of hacking themselves. We don't have a perfect system, and we
16never will. It's still evolving, so let us know if you have suggestions.
17
18Mozilla used to have the concept of "super-review", but `a consensus was
19reached in
202018 <https://groups.google.com/forum/#!topic/mozilla.governance/HHU0h-44NDo>`__
21to retire this process.
22
23Who must review my code?
24------------------------
25
26You must have an approval ("r={{ mediawiki.external('name') }}") from
27the module owner or designated "peer" of the module where the code will
28be checked in. If your code affects several modules, then generally you
29should have an "r={{ mediawiki.external('name') }}" from the owner or
30designated peer of each affected module. We try to be reasonable here,
31so we don't have an absolute rule on when every module owner must
32approve. For example, tree-wide changes such as a change to a string
33class or a change to text that is displayed in many modules generally
34doesn't get reviewed by every module owner.
35
36You may wish to ask others as well.
37
38
39What do reviewers look for?
40---------------------------
41
42A review is focused on a patch's design, implementation, usefulness in
43fixing a stated problem, and fit within its module. A reviewer should be
44someone with domain expertise in the problem area. A reviewer may also
45utilize other areas of his or her expertise and comment on other
46possible improvements. There are no inherent limitations on what
47comments a reviewer might make about improving the code.
48
49Reviewers 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
82How can I tell the status of reviews?
83-------------------------------------
84
85When a patch has passed review you'll see "Accepted" in green at the top
86of a Phabricator revision, under the title. In Bugzilla (which is
87deprecated in favour of Phabricator), this is indicated by "{{
88mediawiki.external('name') }}:review+" in the attachment table in the
89bug report. If it has failed review then you'll see "Needs Revision" in
90red at the top of the revision, or, in Bugzilla, "{{
91mediawiki.external('name') }}:review-". Most of the time that a reviewer
92sets a review flag, they will also add a comment to the bug explaining
93the review.
94