Code reviews are a central part of developing high-quality code for Chromium. All change lists (CLs) must be reviewed.
The general patch, upload, and land process is covered in more detail in the contributing code page. To learn about the code review changes and OWNERS policy changes launched on March 24, 2021, see Mandatory Code Review and Native OWNERS.
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 (more info).
To indicate a positive review, the reviewer provides a
Code-Review +1 in Gerrit, also known as an LGTM (“Looks Good To Me”). A score of “-1” indicates the change should not be submitted as-is.
If you have multiple reviewers, provide a message indicating what you expect from each reviewer. Otherwise people might assume their input is not required or waste time with redundant reviews.
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 do a complete review, but you should be able to give some initial feedback, request more time, or suggest another reviewer.
Use the status field in Gerrit settings to indicate if you‘re away and when you’ll be back.
Don't generally discourage people from sending you code reviews. This includes using a blanket “slow” in your status field.
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. For example, the reviewers in the
//chrome/browser/component_name/OWNERS file will likely be more familiar with code in
//chrome/browser/component_name/sub_component than reviewers in the higher-level
More detail on the owners file format is provided here.
git cl owners command can help find owners. Gerrit also provides this functionality in the Reviewers field of CLs.
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.
The existing owners of a directory approve additions to the list. It is preferable to have many directories, each with a smaller number of specific owners rather than large directories with many owners. Owners should:
Demonstrate excellent judgment, teamwork and ability to uphold Chromium 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 three 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 ninety 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 to the “substantiality” and “recency” requirements.
//third_party should list those most familiar with the library, regardless of how often the code is updated.
If a code owner is not meeting the expectations of owners listed above for more than one quarter (and they are not on a leave during that time), then they may be removed by any co-owner or an owner from the parent directory after a 4-week notice, using the following process:
Refer to the owners plugin 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. firstname.lastname@example.org email@example.com # Only for the frobinator.
* indicates that all committers are owners:
set noparent will stop owner propagation from parent directories. This should be rarely used. If you want to use
set noparent except for IPC related files, please first reach out to firstname.lastname@example.org.
You have to use
set noparent together with a reference to a file that lists the owners for the given use case. Approved use cases are listed in
//build/OWNERS.setnoparent. Owners listed in those files are expected to execute special governance functions such as eng review or ipc security review. Every set of owners should implement their own means of auditing membership. The minimum expectation is that membership in those files is reevaluated on project, or affiliation changes.
In this example, only the eng reviewers are owners:
set noparent file://ENG_REVIEW_OWNERS
per-file directive allows owners to be added that apply only to files matching a pattern. In this example, owners from the parent directory apply, plus one person for some classes of files, and all committers are owners for the readme:
per-file email@example.com per-file firstname.lastname@example.org per-file readme.txt=*
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 File globbing is supported using the [simple path expression format](https://github.com/GerritCodeReview/plugins_code-owners/blob/master/resources/Documentation/path-expressions.md#simplePathExpressions)
Owners-Override +1 label will bypass OWNERS enforcement. Active sheriffs, Release Program Managers, Large Scale Changes, Global Approvers reviewers, Chrome Eng Review members have this capability. The power to use Owners-Override should be restricted as follows:
When you need Owners-Override on sheriffing CLs, please reach out to the Active Sheriffs and Release Program Managers first. If none of them is available, please send an email to email@example.com for help.
Note that Owners-Override by itself is not enough on your own CLs. Where this matters is when you are sheriffing. For example, if you want to revert or disable a test, your Owners-Override on the CL is not enough. You need another committer to LGTM the CL.
For one-off CLs, API owners of
Owners-Override +1 a change to their APIs to avoid waiting for rubberstamp +1s from affected directories' owners. This should only be used for mechanical updates to the affected directories.
If you are making one-off CLs that touch many directories and cannot be handled by the global approvers, you can ask one of Chrome Eng Review members.
You can use the Large Scale Changes process to get approval to bypass OWNERS enforcement for large changes like refactoring, architectural changes, or other repetitive code changes across the whole codebase. This is used for work that span many dozen CLs.
Documentation updates require code review. We may revisit this decision in the future.
For verifiably safe changes like translation files, clean reverts, and clean cherry-picks, we have automation that will vote +1 on the
Bot-Commit label allowing the CL to be submitted without human code-review. Add
Rubber Stamper (firstname.lastname@example.org) to your CL as a reviewer to activate this automation. It will scan the CL after about 1 minute and reply with its verdict.
Bot-Commit votes are not sticky between patchsets and so only add the bot once the CL is finalized.
When combined with the
Owners-Override power, sheriffs can effectively revert and reland on their own.
Rubber Stamper never provides OWNERS approval, by design. It's intended to be used by those who have owners in the directory modified or who are sheriffs. If it provided both code review and OWNERS approval, that would be an abuse vector: that would allow anyone who can create a revert or cherry-pick to land it without any other person being involved (e.g. the silent revert of security patches).
Changes not supported by
Rubber Stamper always need a +1 from another committer.