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 |
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 ``` |
OLD | NEW |