Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(336)

Side by Side Diff: docs/code_reviews.md

Issue 2680013003: Add code review policy document. (Closed)
Patch Set: Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
(Empty)
1 # Code Reviews
2
3 Code reviews are a central part of developing high-quality code for Chromium.
4 All changes must be reviewed.
5
6 The bigger patch-upload-and-land process is covered in more detail the
7 [contributing code](https://www.chromium.org/developers/contributing-code)
8 page.
9
10 # Code review policies
11
12 Ideally the reviewer is someone who is familiar with the area of code you are
13 touching. If you have doubts, look at the git blame for the file and the OWNERS
14 files (see below).
15
16 Any committer can review code, but there must be at least one owner for each
17 directory you are touching. If you have multiple reviewers, make it clear in
18 the message you send requesting review what you expect from each reviewer.
19 Otherwise people might assume their input is not required or waste time with
20 redundant reviews.
21
22 ## Expectations for all reviewers
23
24 * Aim to provide some kind of actionable response within 24 hours of receipt
25 (not counting weekends and holidays). This doesn't mean you have to have
26 done a complete review, but you should be able to give some initial
27 feedback, request more time, or suggest another reviewer.
28
29 * It can be nice to indicate if you're away in your name in the code review
30 tool. If you do this, indicate when you'll be back.
31
32 * Don't generally discourage people from sending you code reviews. This
33 includes writing a blanket ("slow") after your name in the review tool.
34
35 ## OWNERS files
36
37 In various directories there are files named "OWNERS" that list the email
38 addresses of people qualified to review changes in that directory. You must
39 get a positive review from an owner of each directory your change touches.
40
41 Owners files are recursive, so each file also applies to its subdirectories. If
42 an owner file contains the text "set noparent" it will stop owner propagation
43 from parent directories.
44
45 Tip: The "git cl owners" command can help find owners.
46
47 While owners must approve all patches, any committer can contribute to the
48 review. In some directories the owners can be overloaded or there might be
49 people not listed as owners who are more familiar with the low-level code in
50 question. In these cases it's common to request a low-level review from an
51 appropriate person, and then request a high-level owner review once that's
52 complete. As always, be clear what you expect of each reviewer to avoid
53 duplicated work.
54
55 ### Expectations for owners
56
57 * OWNERS files should contain only people actively reviewing code in that
58 directory.
59
60 * If you aren't doing code reviews in a directory, remove yourself. Don't
61 try to discourage people from sending reviews in comments in the OWNERS
62 file. This includes writing "slow" or "emeritus" after your name.
63
64 * If the code review load is unsustainable, work to expand the number of
65 owners.
66
67 ## TBR ("To Be Reviewed")
68
69 "TBR" is our mechanism for post-commit review. It should be used rarely and
70 only in cases where a review is unnecessary or as described below. The most
71 common use of TBR is to revert patches that broke the build.
72
73 TBR does not mean "no review." A reviewer on a TBRed on a change should still
74 review the change. If there comments after landing the author is still
75 obligated to address them in a followup patch.
76
77 Do not use TBR just because a change is urgent or the reviewer is being slow.
78 Work to contact a reviewer who can do your review in a timely manner.
79
80 To send a change TBR, annotate the description and send email like normal.
81 Otherwise the reviewer won't know to review the patch.
82
83 * Add the reviewer's email address in the code review tool's reviewer field
84 like normal.
85
86 * Add a line "TBR=<reviewer's email>" to the bottom of the change list
87 description.
88
89 * Push the "send mail" button.
90
91 ### TBR-ing certain types of mechanical changes
92
93 Sometimes you might do something that affects many callers in different
94 directories. For example, adding a parameter to a common function in //base.
95 If the updates to the callers is mechanical, you can:
96
97 * Get a normal owner of the lower-level directory (in this example, //base)
98 you're changing to do a proper review of those changes.
99
100 * Get _somebody_ to review the downstream changes. This is often the same
101 person from the previous step but could be somebody else.
102
103 * Add the owners of the affected downstream directories as TBR.
104
105 This process ensures that all code is reviewed prior to checkin and that the
106 concept of the change is reviewed by a qualified person, but you don't have to
107 track down many individual owners for trivial changes to their directories.
108
109 ### TBR-ing documentation updates
110
111 You can TBR documentation updates. Documentation means markdown files, text
112 documents, and high-level comments in code. At finer levels of detail, comments
113 in source files become more like code and should be reviewed normally (not
114 using TBR). Non-TBR-able stuff includes things like function contracts and most
115 comments inside functions.
116
117 * Use your best judgement. If you're changing something very important,
118 tricky, or something you may not be very familiar with, ask for the code
119 review up-front.
120
121 * Don't TBR changes to policy documents like the style guide or this document.
122
123 * Don't mix unrelated documentation updates with code changes.
124
125 * Be sure to actually send out the email for the code review. If you get one,
126 please actually read the changes. Even the best writers have editors and
127 checking prose for clarity is much faster than checking code for
128 correctness.
OLDNEW
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698