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

Side by Side Diff: docs/code_reviews.md

Issue 2932013002: Add examples for some scenarios in the code review docs. (Closed)
Patch Set: format Created 3 years, 6 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
(...skipping 25 matching lines...) Expand all
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
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
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 ```
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