Chromium Code Reviews| 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 |