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

Side by Side Diff: docs/code_reviews.md

Issue 2687063002: Add more information on owners to the code review docs. (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
1 # Code Reviews 1 # Code Reviews
2 2
3 Code reviews are a central part of developing high-quality code for Chromium. 3 Code reviews are a central part of developing high-quality code for Chromium.
4 All changes must be reviewed. 4 All changes must be reviewed.
5 5
6 The bigger patch-upload-and-land process is covered in more detail the 6 The bigger patch-upload-and-land process is covered in more detail the
7 [contributing code](https://www.chromium.org/developers/contributing-code) 7 [contributing code](https://www.chromium.org/developers/contributing-code)
8 page. 8 page.
9 9
10 # Code review policies 10 # Code review policies
11 11
12 Ideally the reviewer is someone who is familiar with the area of code you are 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 13 touching. Any committer can review code, but an owner must provide a review
14 files (see below). 14 for each directory you are touching. If you have doubts, look at the git blame
15 for the file and the `OWNERS` files (see below).
15 16
16 Any committer can review code, but there must be at least one owner for each 17 To indicate a positive review, the reviewer types "LGTM" (case insensitive)
17 directory you are touching. If you have multiple reviewers, make it clear in 18 into a comment on the code review. This stands for "Looks Good To Me." The text
18 the message you send requesting review what you expect from each reviewer. 19 "not LGTM" will cancel out a previous positive review.
19 Otherwise people might assume their input is not required or waste time with
20 redundant reviews.
21 20
22 ## Expectations for all reviewers 21 If you have multiple reviewers, make it clear in the message you send
22 requesting review what you expect from each reviewer. Otherwise people might
23 assume their input is not required or waste time with redundant reviews.
24
25 #### Expectations for all reviewers
23 26
24 * Aim to provide some kind of actionable response within 24 hours of receipt 27 * 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 28 (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 29 done a complete review, but you should be able to give some initial
27 feedback, request more time, or suggest another reviewer. 30 feedback, request more time, or suggest another reviewer.
28 31
29 * It can be nice to indicate if you're away in your name in the code review 32 * 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. 33 tool. If you do this, indicate when you'll be back.
31 34
32 * Don't generally discourage people from sending you code reviews. This 35 * Don't generally discourage people from sending you code reviews. This
33 includes writing a blanket ("slow") after your name in the review tool. 36 includes writing a blanket ("slow") after your name in the review tool.
34 37
35 ## OWNERS files 38 ## OWNERS files
36 39
37 In various directories there are files named "OWNERS" that list the email 40 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 41 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. 42 get a positive review from an owner of each directory your change touches.
40 43
41 Owners files are recursive, so each file also applies to its subdirectories. If 44 Owners files are recursive, so each file also applies to its subdirectories.
42 an owner file contains the text "set noparent" it will stop owner propagation 45 It's generally best to pick more specific owners. People listed in higher-level
43 from parent directories. 46 directories may have less experience with the code in question. More detail on
47 the owners file format is provided in the "More information" section below.
44 48
45 Tip: The "git cl owners" command can help find owners. 49 *Tip:* The `git cl owners` command can help find owners.
46 50
47 While owners must approve all patches, any committer can contribute to the 51 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 52 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 53 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 54 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 55 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 56 complete. As always, be clear what you expect of each reviewer to avoid
53 duplicated work. 57 duplicated work.
54 58
55 ### Expectations for owners 59 Owners do not have to pick other owners for reviews. Since they should already
60 be familiar with the code in question, a thorough review from any appropriate
61 committer is sufficient.
56 62
57 * OWNERS files should contain only people actively reviewing code in that 63 #### Expectations of owners
64
65 The existing owners of a directory approve additions to the list. It is
66 preferrable to have many directories, each with a smaller number of specific
67 owners rather than large directories with many owners. Owners must:
68
69 * Demonstrate excellent judgment, teamwork and ability to uphold Chrome
70 development principles.
71
72 * Be already acting as an owner, providing high-quality reviews and design
73 feedback
74
75 * Be a Chromium project member with full commit access of at least 6
76 months tenure.
77
78 * Have submitted a substantial number of non-trivial changes to the affected
58 directory. 79 directory.
59 80
60 * If you aren't doing code reviews in a directory, remove yourself. Don't 81 * Have committed or reviewed substantial work to the affected directory
61 try to discourage people from sending reviews in comments in the OWNERS 82 within the last 90 days.
62 file. This includes writing "slow" or "emeritus" after your name.
63 83
64 * If the code review load is unsustainable, work to expand the number of 84 * Have the bandwidth to contribute to reviews in a timely manner. If the load
65 owners. 85 is unsustainable, work to expand the number of owners. Don't try to
86 discourage people from sending reviews, including writing "slow" or
87 "emeritus" after your name.
88
89 Seldom-updated directories may have exceptions. Directories in `third_party`
90 should list those most familiar with the library.
66 91
67 ## TBR ("To Be Reviewed") 92 ## TBR ("To Be Reviewed")
68 93
69 "TBR" is our mechanism for post-commit review. It should be used rarely and 94 "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 95 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. 96 common use of TBR is to revert patches that broke the build.
72 97
73 TBR does not mean "no review." A reviewer on a TBRed on a change should still 98 TBR does not mean "no review." A reviewer TBR-ed on a change should still
74 review the change. If there comments after landing the author is still 99 review the change. If there comments after landing the author is obligated to
75 obligated to address them in a followup patch. 100 address them in a followup patch.
76 101
77 Do not use TBR just because a change is urgent or the reviewer is being slow. 102 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. 103 Contact the reviewer directly or find somebody.
79 104
80 To send a change TBR, annotate the description and send email like normal. 105 To send a change TBR, annotate the description and send email like normal.
81 Otherwise the reviewer won't know to review the patch. 106 Otherwise the reviewer won't know to review the patch.
82 107
83 * Add the reviewer's email address in the code review tool's reviewer field 108 * Add the reviewer's email address in the code review tool's reviewer field
84 like normal. 109 like normal.
85 110
86 * Add a line "TBR=<reviewer's email>" to the bottom of the change list 111 * Add a line "TBR=<reviewer's email>" to the bottom of the change list
87 description. 112 description.
88 113
89 * Push the "send mail" button. 114 * Push the "send mail" button.
90 115
91 ### TBR-ing certain types of mechanical changes 116 ### TBR-ing certain types of mechanical changes
92 117
93 Sometimes you might do something that affects many callers in different 118 Sometimes you might do something that affects many callers in different
94 directories. For example, adding a parameter to a common function in //base. 119 directories. For example, adding a parameter to a common function in //base.
95 If the updates to the callers is mechanical, you can: 120 If the updates to the callers is mechanical, you can:
96 121
97 * Get a normal owner of the lower-level directory (in this example, //base) 122 * Get a normal owner of the lower-level directory you're changing (in this
98 you're changing to do a proper review of those changes. 123 example, `//base`) to do a proper review of those changes.
99 124
100 * Get _somebody_ to review the downstream changes. This is often the same 125 * Get _somebody_ to review the downstream changes. This is often the same
101 person from the previous step but could be somebody else. 126 person from the previous step but could be somebody else.
102 127
103 * Add the owners of the affected downstream directories as TBR. 128 * Add the owners of the affected downstream directories as TBR.
104 129
105 This process ensures that all code is reviewed prior to checkin and that the 130 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 131 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. 132 track down many individual owners for trivial changes to their directories.
108 133
109 ### TBR-ing documentation updates 134 ### TBR-ing documentation updates
110 135
111 You can TBR documentation updates. Documentation means markdown files, text 136 You can TBR documentation updates. Documentation means markdown files, text
112 documents, and high-level comments in code. At finer levels of detail, comments 137 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 138 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 139 using TBR). Non-TBR-able stuff includes things like function contracts and most
115 comments inside functions. 140 comments inside functions.
116 141
117 * Use your best judgement. If you're changing something very important, 142 * Use good judgement. If you're changing something very important, tricky,
118 tricky, or something you may not be very familiar with, ask for the code 143 or something you may not be very familiar with, ask for the code review
119 review up-front. 144 up-front.
120 145
121 * Don't TBR changes to policy documents like the style guide or this document. 146 * Don't TBR changes to policy documents like the style guide or this document.
122 147
123 * Don't mix unrelated documentation updates with code changes. 148 * Don't mix unrelated documentation updates with code changes.
124 149
125 * Be sure to actually send out the email for the code review. If you get one, 150 * 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 151 please actually read the changes.
127 checking prose for clarity is much faster than checking code for 152
128 correctness. 153 ## More information
154
155 ### OWNERS file details
156
157 Refer to the [source code](https://chromium.googlesource.com/chromium/tools/depo t_tools/+/master/owners.py)
158 for all details on the file format.
159
160 This example indicates that two people are owners, in addition to any owners
161 from the parent directory. `git cl owners` will list the comment after an
162 owner address, so this is a good place to include restrictions or special
163 instructions.
164 ```
165 # You can include comments like this.
166 a@chromium.org
167 b@chromium.org # Only for the frobinator.
168 ```
169
170 A `*` indicates that all committers are owners:
171 ```
172 *
173 ```
174
175 The text `set noparent` it will stop owner propagation from parent directories.
176 This is used for specialized code. In this example, only the two listed people
177 are owners:
178 ```
179 set noparent
180 a@chromium.org
181 b@chromium.org
182 ```
183
184 The `per-file` directive allows owners to be added that apply only to files
185 matching a pattern. In this example, owners from the parent directiory
186 apply, plus one person for some classes of files, and all committers are
187 owners for the readme:
188 ```
189 per-file foo_bar.cc=a@chromium.org
190 per-file foo.*=a@chromium.org
191
192 per-file readme.txt=*
193 ```
194
195 Other `OWNERS` files can be included by reference by listing the path to the
196 file with `file://...`. This example indicates that only the people listed in
197 `//ipc/SECURITY_OWNERS` can review the messages files:
198 ```
199 per-file *_messages*.h=set noparent
200 per-file *_messages*.h=file://ipc/SECURITY_OWNERS
201 ```
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