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