diff options
Diffstat (limited to 'docs/contributing/Code_Review_FAQ.rst')
-rw-r--r-- | docs/contributing/Code_Review_FAQ.rst | 93 |
1 files changed, 93 insertions, 0 deletions
diff --git a/docs/contributing/Code_Review_FAQ.rst b/docs/contributing/Code_Review_FAQ.rst new file mode 100644 index 0000000000..18fe85a6e2 --- /dev/null +++ b/docs/contributing/Code_Review_FAQ.rst @@ -0,0 +1,93 @@ +Code Review FAQ +=============== + +What is the purpose of code review? +----------------------------------- + +Code review is our basic mechanism for validating the design and +implementation of patches. It also helps us maintain a level of +consistency in design and implementation practices across the many +hackers and among the various modules of Mozilla. + +Of course, code review doesn't happen instantaneously, and so there is +some latency built into the system. We're always looking for ways to +reduce the wait, while simultaneously allowing reviewers to do a good +chunk of hacking themselves. We don't have a perfect system, and we +never will. It's still evolving, so let us know if you have suggestions. + +Mozilla used to have the concept of "super-review", but `a consensus was +reached in +2018 <https://groups.google.com/forum/#!topic/mozilla.governance/HHU0h-44NDo>`__ +to retire this process. + +Who must review my code? +------------------------ + +You must have an approval ("r={{ mediawiki.external('name') }}") from +the module owner or designated "peer" of the module where the code will +be checked in. If your code affects several modules, then generally you +should have an "r={{ mediawiki.external('name') }}" from the owner or +designated peer of each affected module. We try to be reasonable here, +so we don't have an absolute rule on when every module owner must +approve. For example, tree-wide changes such as a change to a string +class or a change to text that is displayed in many modules generally +doesn't get reviewed by every module owner. + +You may wish to ask others as well. + + +What do reviewers look for? +--------------------------- + +A review is focused on a patch's design, implementation, usefulness in +fixing a stated problem, and fit within its module. A reviewer should be +someone with domain expertise in the problem area. A reviewer may also +utilize other areas of his or her expertise and comment on other +possible improvements. There are no inherent limitations on what +comments a reviewer might make about improving the code. + +Reviewers will probably look at the following areas of the code: + +- “goal” review: is the issue being fixed actually a bug? Does the + patch fix the fundamental problem? +- API/design review. Because APIs define the interactions between + modules, they need special care. Review is especially important to + keep APIs balanced and targeted, and not too specific or + overdesigned. There are a `WebIDL review + checklist <https://wiki.mozilla.org/WebAPI/WebIDL_Review_Checklist>`__. + There are also templates for emails that should be sent when APIs are + going to be exposed to the Web and general guidance around naming on + `this wiki + page <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>`__. +- Maintainability review. Code which is unreadable is impossible to + maintain. If the reviewer has to ask questions about the purpose of a + piece of code, then it is probably not documented well enough. Does + the code follow the :ref:`Coding style` ? Be careful when + reviewing code using modern C++ features like auto. +- Security review. Does the design use security concepts such as input + sanitizers, wrappers, and other techniques? Does this code need + additional security testing such as fuzz-testing or static analysis? +- Integration review. Does this code work properly with other modules? + Is it localized properly? Does it have server dependencies? Does it + have user documentation? +- Testing review. Are there tests for correct function? Are there tests + for error conditions and incorrect inputs which could happen during + operation? +- Performance review. Has this code been profiled? Are you sure it's + not negatively affecting performance of other code? +- License review. Does the code follow the `code licensing + rules <http://www.mozilla.org/hacking/committer/committers-agreement.pdf>`__? + + +How can I tell the status of reviews? +------------------------------------- + +When a patch has passed review you'll see "Accepted" in green at the top +of a Phabricator revision, under the title. In Bugzilla (which is +deprecated in favour of Phabricator), this is indicated by "{{ +mediawiki.external('name') }}:review+" in the attachment table in the +bug report. If it has failed review then you'll see "Needs Revision" in +red at the top of the revision, or, in Bugzilla, "{{ +mediawiki.external('name') }}:review-". Most of the time that a reviewer +sets a review flag, they will also add a comment to the bug explaining +the review. |