brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 1 | # Code Reviews |
| 2 | |
| 3 | Code reviews are a central part of developing high-quality code for Chromium. |
| 4 | All changes must be reviewed. |
| 5 | |
Daniel Cheng | 6bffde0 | 2020-06-12 19:10:45 | [diff] [blame^] | 6 | The general patch, upload, and land process is covered in more detail in the |
| 7 | [contributing code](contributing.md) page. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 8 | |
| 9 | # Code review policies |
| 10 | |
| 11 | Ideally the reviewer is someone who is familiar with the area of code you are |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 12 | touching. Any committer can review code, but an owner must provide a review |
| 13 | for each directory you are touching. If you have doubts, look at the git blame |
| 14 | for the file and the `OWNERS` files (see below). |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 15 | |
Michael Giuffrida | af36705 | 2018-03-22 20:22:34 | [diff] [blame] | 16 | To indicate a positive review, the reviewer provides a "Code-Review +1" in |
| 17 | Gerrit, also known as an LGTM ("Looks Good To Me"). A score of "-1" indicates |
| 18 | the change should not be submitted as-is. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 19 | |
Michael Giuffrida | af36705 | 2018-03-22 20:22:34 | [diff] [blame] | 20 | If you have multiple reviewers, provide a message indicating what you expect |
| 21 | from each reviewer. Otherwise people might assume their input is not required |
| 22 | or waste time with redundant reviews. |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 23 | |
Annie Sullivan | d04212e7 | 2017-10-19 21:11:32 | [diff] [blame] | 24 | Please also read [Respectful Changes](cl_respect.md) and |
| 25 | [Respectful Code Reviews](cr_respect.md). |
| 26 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 27 | #### Expectations for all reviewers |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 28 | |
| 29 | * Aim to provide some kind of actionable response within 24 hours of receipt |
Michael Giuffrida | af36705 | 2018-03-22 20:22:34 | [diff] [blame] | 30 | (not counting weekends and holidays). This doesn't mean you have to do a |
| 31 | complete review, but you should be able to give some initial feedback, |
| 32 | request more time, or suggest another reviewer. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 33 | |
Michael Giuffrida | af36705 | 2018-03-22 20:22:34 | [diff] [blame] | 34 | * Use the status field in Gerrit settings to indicate if you're away and when |
Mike Frysinger | 7b15bde | 2018-05-15 09:28:05 | [diff] [blame] | 35 | you'll be back. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 36 | |
| 37 | * Don't generally discourage people from sending you code reviews. This |
Michael Giuffrida | af36705 | 2018-03-22 20:22:34 | [diff] [blame] | 38 | includes using a blanket "slow" in your status field. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 39 | |
| 40 | ## OWNERS files |
| 41 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 42 | In various directories there are files named `OWNERS` that list the email |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 43 | addresses of people qualified to review changes in that directory. You must |
| 44 | get a positive review from an owner of each directory your change touches. |
| 45 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 46 | Owners files are recursive, so each file also applies to its subdirectories. |
| 47 | It's generally best to pick more specific owners. People listed in higher-level |
thestig | 9208d8ba | 2017-06-09 22:05:32 | [diff] [blame] | 48 | directories may have less experience with the code in question. For example, |
| 49 | the reviewers in the `//chrome/browser/component_name/OWNERS` file will likely |
| 50 | be more familiar with code in `//chrome/browser/component_name/sub_component` |
| 51 | than reviewers in the higher-level `//chrome/OWNERS` file. |
| 52 | |
| 53 | More detail on the owners file format is provided in the "More information" |
| 54 | section below. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 55 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 56 | *Tip:* The `git cl owners` command can help find owners. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 57 | |
| 58 | While owners must approve all patches, any committer can contribute to the |
| 59 | review. In some directories the owners can be overloaded or there might be |
| 60 | people not listed as owners who are more familiar with the low-level code in |
| 61 | question. In these cases it's common to request a low-level review from an |
| 62 | appropriate person, and then request a high-level owner review once that's |
| 63 | complete. As always, be clear what you expect of each reviewer to avoid |
| 64 | duplicated work. |
| 65 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 66 | Owners do not have to pick other owners for reviews. Since they should already |
| 67 | be familiar with the code in question, a thorough review from any appropriate |
| 68 | committer is sufficient. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 69 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 70 | #### Expectations of owners |
| 71 | |
| 72 | The existing owners of a directory approve additions to the list. It is |
Wei-Yin Chen (陳威尹) | 681bc32 | 2017-07-20 01:55:11 | [diff] [blame] | 73 | preferable to have many directories, each with a smaller number of specific |
Dirk Pranke | 4f9740c | 2018-10-17 03:01:06 | [diff] [blame] | 74 | owners rather than large directories with many owners. Owners should: |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 75 | |
| 76 | * Demonstrate excellent judgment, teamwork and ability to uphold Chrome |
| 77 | development principles. |
| 78 | |
| 79 | * Be already acting as an owner, providing high-quality reviews and design |
Dirk Pranke | 4f9740c | 2018-10-17 03:01:06 | [diff] [blame] | 80 | feedback. |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 81 | |
Dirk Pranke | 4f9740c | 2018-10-17 03:01:06 | [diff] [blame] | 82 | * Be a Chromium project member with full commit access of at least three |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 83 | months tenure. |
| 84 | |
| 85 | * Have submitted a substantial number of non-trivial changes to the affected |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 86 | directory. |
| 87 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 88 | * Have committed or reviewed substantial work to the affected directory |
Dirk Pranke | 4f9740c | 2018-10-17 03:01:06 | [diff] [blame] | 89 | within the last ninety days. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 90 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 91 | * Have the bandwidth to contribute to reviews in a timely manner. If the load |
| 92 | is unsustainable, work to expand the number of owners. Don't try to |
| 93 | discourage people from sending reviews, including writing "slow" or |
| 94 | "emeritus" after your name. |
| 95 | |
Dirk Pranke | 4f9740c | 2018-10-17 03:01:06 | [diff] [blame] | 96 | The above are guidelines more than they are hard rules, and exceptions are |
| 97 | okay as long as there is a consensus by the existing owners for them. |
| 98 | For example, seldom-updated directories may have exceptions to the |
| 99 | "substantiality" and "recency" requirements. Directories in `third_party` |
| 100 | should list those most familiar with the library, regardless of how often |
| 101 | the code is updated. |
brettw | 40e953e | 2017-02-08 17:49:28 | [diff] [blame] | 102 | |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 103 | ### OWNERS file details |
| 104 | |
| 105 | Refer to the [source code](https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/owners.py) |
thestig | 9208d8ba | 2017-06-09 22:05:32 | [diff] [blame] | 106 | for all details on the file format. |
brettw | 2019b9e | 2017-02-09 06:40:20 | [diff] [blame] | 107 | |
| 108 | This example indicates that two people are owners, in addition to any owners |
| 109 | from the parent directory. `git cl owners` will list the comment after an |
| 110 | owner address, so this is a good place to include restrictions or special |
| 111 | instructions. |
| 112 | ``` |
| 113 | # You can include comments like this. |
|
|