OLD | NEW |
---|---|
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 |
(...skipping 25 matching lines...) Expand all Loading... | |
36 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. |
37 | 37 |
38 ## OWNERS files | 38 ## OWNERS files |
39 | 39 |
40 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 |
41 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 |
42 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. |
43 | 43 |
44 Owners files are recursive, so each file also applies to its subdirectories. | 44 Owners files are recursive, so each file also applies to its subdirectories. |
45 It's generally best to pick more specific owners. People listed in higher-level | 45 It's generally best to pick more specific owners. People listed in higher-level |
46 directories may have less experience with the code in question. More detail on | 46 directories may have less experience with the code in question. For example, |
47 the owners file format is provided in the "More information" section below. | 47 the reviewers in the `//chrome/browser/component_name/OWNERS` file will likely |
48 be more familiar with code in `//chrome/browser/component_name/sub_component` | |
49 than reviewers in the higher-level `//chrome/OWNERS` file. | |
50 | |
51 More detail on the owners file format is provided in the "More information" | |
52 section below. | |
48 | 53 |
49 *Tip:* The `git cl owners` command can help find owners. | 54 *Tip:* The `git cl owners` command can help find owners. |
50 | 55 |
51 While owners must approve all patches, any committer can contribute to the | 56 While owners must approve all patches, any committer can contribute to the |
52 review. In some directories the owners can be overloaded or there might be | 57 review. In some directories the owners can be overloaded or there might be |
53 people not listed as owners who are more familiar with the low-level code in | 58 people not listed as owners who are more familiar with the low-level code in |
54 question. In these cases it's common to request a low-level review from an | 59 question. In these cases it's common to request a low-level review from an |
55 appropriate person, and then request a high-level owner review once that's | 60 appropriate person, and then request a high-level owner review once that's |
56 complete. As always, be clear what you expect of each reviewer to avoid | 61 complete. As always, be clear what you expect of each reviewer to avoid |
57 duplicated work. | 62 duplicated work. |
(...skipping 44 matching lines...) Expand 10 before | Expand all | Expand 10 after Loading... | |
102 Do not use TBR just because a change is urgent or the reviewer is being slow. | 107 Do not use TBR just because a change is urgent or the reviewer is being slow. |
103 Contact the reviewer directly or find somebody. | 108 Contact the reviewer directly or find somebody. |
104 | 109 |
105 To send a change TBR, annotate the description and send email like normal. | 110 To send a change TBR, annotate the description and send email like normal. |
106 Otherwise the reviewer won't know to review the patch. | 111 Otherwise the reviewer won't know to review the patch. |
107 | 112 |
108 * Add the reviewer's email address in the code review tool's reviewer field | 113 * Add the reviewer's email address in the code review tool's reviewer field |
109 like normal. | 114 like normal. |
110 | 115 |
111 * Add a line "TBR=<reviewer's email>" to the bottom of the change list | 116 * Add a line "TBR=<reviewer's email>" to the bottom of the change list |
112 description. | 117 description. e.g. `TBR=reviewer1@chromium.org,reviewer2@chromium.org` |
118 | |
119 * Type a message so that the owners in the TBR list can understand who is | |
120 responsible for reviewing what, as part of their post-commit review | |
121 responsibility. e.g. | |
122 ``` | |
brettw
2017/06/09 15:31:17
I'm not sure gitiles will do what you want here, b
Lei Zhang
2017/06/09 21:52:50
Seems to work: https://chromium.googlesource.com/c
| |
123 TBRing reviewers: | |
124 reviewer1: Please review changes to foo/ | |
125 reviewer2: Please review changes to bar/ | |
126 ``` | |
113 | 127 |
114 * Push the "send mail" button. | 128 * Push the "send mail" button. |
115 | 129 |
116 ### TBR-ing certain types of mechanical changes | 130 ### TBR-ing certain types of mechanical changes |
117 | 131 |
118 Sometimes you might do something that affects many callers in different | 132 Sometimes you might do something that affects many callers in different |
119 directories. For example, adding a parameter to a common function in //base. | 133 directories. For example, adding a parameter to a common function in |
120 If the updates to the callers is mechanical, you can: | 134 `//base`, with callers in `//chrome/browser/foo`, `//net/bar`, and many other |
135 directories. If the updates to the callers is mechanical, you can: | |
121 | 136 |
122 * Get a normal owner of the lower-level directory you're changing (in this | 137 * Get a normal owner of the lower-level code you're changing (in this |
123 example, `//base`) to do a proper review of those changes. | 138 example, the function in `//base`) to do a proper review of those changes. |
124 | 139 |
125 * Get _somebody_ to review the downstream changes. This is often the same | 140 * Get _somebody_ to review the downstream changes made to the callers as a |
126 person from the previous step but could be somebody else. | 141 result of the `//base` change. This is often the same person from the |
142 previous step but could be somebody else. | |
127 | 143 |
128 * Add the owners of the affected downstream directories as TBR. | 144 * Add the owners of the affected downstream directories as TBR. (In this |
145 example, reviewers from `//chrome/browser/foo/OWNERS`, `//net/bar/OWNERS`, | |
146 etc.) | |
129 | 147 |
130 This process ensures that all code is reviewed prior to checkin and that the | 148 This process ensures that all code is reviewed prior to checkin and that the |
131 concept of the change is reviewed by a qualified person, but you don't have to | 149 concept of the change is reviewed by a qualified person, but you don't have to |
132 track down many individual owners for trivial changes to their directories. | 150 wait for many individual owners to review trivial changes to their directories. |
133 | 151 |
134 ### TBR-ing documentation updates | 152 ### TBR-ing documentation updates |
135 | 153 |
136 You can TBR documentation updates. Documentation means markdown files, text | 154 You can TBR documentation updates. Documentation means markdown files, text |
137 documents, and high-level comments in code. At finer levels of detail, comments | 155 documents, and high-level comments in code. At finer levels of detail, comments |
138 in source files become more like code and should be reviewed normally (not | 156 in source files become more like code and should be reviewed normally (not |
139 using TBR). Non-TBR-able stuff includes things like function contracts and most | 157 using TBR). Non-TBR-able stuff includes things like function contracts and most |
140 comments inside functions. | 158 comments inside functions. |
141 | 159 |
142 * Use good judgement. If you're changing something very important, tricky, | 160 * Use good judgement. If you're changing something very important, tricky, |
143 or something you may not be very familiar with, ask for the code review | 161 or something you may not be very familiar with, ask for the code review |
144 up-front. | 162 up-front. |
145 | 163 |
146 * Don't TBR changes to policy documents like the style guide or this document. | 164 * Don't TBR changes to policy documents like the style guide or this document. |
147 | 165 |
148 * Don't mix unrelated documentation updates with code changes. | 166 * Don't mix unrelated documentation updates with code changes. |
149 | 167 |
150 * Be sure to actually send out the email for the code review. If you get one, | 168 * Be sure to actually send out the email for the code review. If you get one, |
151 please actually read the changes. | 169 please actually read the changes. |
152 | 170 |
153 ## More information | 171 ## More information |
154 | 172 |
155 ### OWNERS file details | 173 ### OWNERS file details |
156 | 174 |
157 Refer to the [source code](https://chromium.googlesource.com/chromium/tools/depo t_tools/+/master/owners.py) | 175 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. | 176 for all details on the file format. |
159 | 177 |
160 This example indicates that two people are owners, in addition to any owners | 178 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 | 179 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 | 180 owner address, so this is a good place to include restrictions or special |
163 instructions. | 181 instructions. |
164 ``` | 182 ``` |
165 # You can include comments like this. | 183 # You can include comments like this. |
166 a@chromium.org | 184 a@chromium.org |
167 b@chromium.org # Only for the frobinator. | 185 b@chromium.org # Only for the frobinator. |
168 ``` | 186 ``` |
(...skipping 23 matching lines...) Expand all Loading... | |
192 per-file readme.txt=* | 210 per-file readme.txt=* |
193 ``` | 211 ``` |
194 | 212 |
195 Other `OWNERS` files can be included by reference by listing the path to the | 213 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 | 214 file with `file://...`. This example indicates that only the people listed in |
197 `//ipc/SECURITY_OWNERS` can review the messages files: | 215 `//ipc/SECURITY_OWNERS` can review the messages files: |
198 ``` | 216 ``` |
199 per-file *_messages*.h=set noparent | 217 per-file *_messages*.h=set noparent |
200 per-file *_messages*.h=file://ipc/SECURITY_OWNERS | 218 per-file *_messages*.h=file://ipc/SECURITY_OWNERS |
201 ``` | 219 ``` |
OLD | NEW |