Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 1 | # Contributing to Chromium |
| 2 | |
| 3 | This page assumes a working Chromium [checkout and build][checkout-and-build]. |
| 4 | Note that a full Chromium checkout includes external repositories with their |
| 5 | own workflows for contributing, such as [v8][v8-dev-guide] and |
| 6 | [Skia][skia-dev-guide]. Similarly, ChromiumOS, which includes Chromium as a |
| 7 | subrepository, has its own [development workflow][cros-dev-guide]. |
| 8 | |
| 9 | [TOC] |
| 10 | |
| 11 | ## Related resources |
| 12 | |
| 13 | - [Life of a Chromium Developer][life-of-a-chromium-developer], which is mostly |
| 14 | up-to-date. |
Toby Huang | 5105f81 | 2019-08-08 23:47:57 | [diff] [blame] | 15 | - [Tutorial][noms-tutorial] by committer emeritus [email protected]. |
| 16 | - [Commit Checklist][commit-checklist], a useful checklist to go through before |
| 17 | submitting each CL on Gerrit. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 18 | |
| 19 | ## Communicate |
| 20 | |
| 21 | When writing a new feature or fixing an existing bug, get a second opinion |
| 22 | before going too far. If it's a new feature idea, propose it to the appropriate |
| 23 | [discussion group][discussion-groups]. If it's in the existing code base, talk |
| 24 | to some of the folks in the "OWNERS" file (see [code review |
| 25 | policies][code-reviews] for more) for the code being changed. |
| 26 | |
| 27 | - If a change needs further context outside the CL, it should be tracked in the |
| 28 | [bug system][crbug]. Bugs are the right place for long histories, discussion |
| 29 | and debate, attaching screenshots, and linking to other associated bugs. Bugs |
| 30 | are unnecessary for changes isolated enough to need none of these. |
| 31 | - If there isn't a bug and there should be one, please [file a new |
| 32 | bug][crbug-new]. |
| 33 | - Just because there is a bug in the bug system doesn't necessarily mean that a |
| 34 | patch will be accepted. |
| 35 | |
John Abd-El-Malek | 27e1cf0 | 2019-12-18 17:35:18 | [diff] [blame] | 36 | ## Design Documents |
| 37 | Any nontrivial technical effort that will significantly impact Chromium should |
| 38 | have a design doc ([template][design-doc-template]). Specifically, we require |
| 39 | design docs in the following cases: |
| 40 | - When writing code that will have a large impact on Chromium as a whole, e.g. |
| 41 | when you are changing code in Chromium's critical path (page loading, |
| 42 | rendering). |
| 43 | - When beginning a large technical undertaking that should be documented for |
| 44 | historical reasons (>1 person-month of work can be used as a general guideline). |
| 45 | |
| 46 | Send public design docs to |
| 47 | [[email protected]][chromium-design-docs]. Google internal Chrome |
| 48 | design docs should follow the process at |
| 49 | [go/chrome-dd-review-process][chrome-dd-review-process]. |
| 50 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 51 | ## Legal stuff |
| 52 | |
Dirk Pranke | b12d6703 | 2022-01-13 17:19:21 | [diff] [blame] | 53 | All contributors must have valid Gerrit/Google accounts (which means you must |
| 54 | be [old enough to manage your own |
| 55 | account](https://support.google.com/accounts/answer/1350409)) and complete the |
| 56 | contributor license agreement. |
| 57 | |
| 58 | For individual contributors, please complete the [Individual Contributor License |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 59 | Agreement][individual-cla] online. Corporate contributors must fill out the |
| 60 | [Corporate Contributor License Agreement][corporate-cla] and send it to us as |
| 61 | described on that page. |
| 62 | |
| 63 | ### First-time contributors |
| 64 | |
| 65 | Add your (or your organization's) name and contact info to the AUTHORS file for |
| 66 | [Chromium][cr-authors] or [Chromium OS][cros-authors]. Please include this as |
| 67 | part of your first patch and not as a separate standalone patch. |
| 68 | |
| 69 | ### External contributor checklist for reviewers |
| 70 | |
| 71 | Before LGTMing a change from a non-chromium.org address, ensure that the |
| 72 | contribution can be accepted: |
| 73 | |
| 74 | - Definition: The "author" is the email address that owns the code review |
| 75 | request on <https://chromium-review.googlesource.com> |
| 76 | - Ensure the author is already listed in [AUTHORS][cr-authors]. In some cases, the |
| 77 | author's company might have a wildcard rule (e.g. \*@google.com). |
| 78 | - If the author or their company is not listed, the CL should include a new |
| 79 | AUTHORS entry. |
| 80 | - Ensure the new entry is reviewed by a reviewer who works for Google. |
Vincent Scheib | 04582d84 | 2020-04-24 18:41:36 | [diff] [blame] | 81 | - Contributor License Agreement can be verified by Googlers at http://go/cla. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 82 | - If there is a corporate CLA for the author's company, it must list the |
| 83 | person explicitly (or the list of authorized contributors must say |
| 84 | something like "All employees"). If the author is not on their company's |
| 85 | roster, do not accept the change. |
| 86 | |
| 87 | ## Initial git setup |
| 88 | |
Allen Li | 14a3888 | 2025-03-24 21:01:13 | [diff] [blame] | 89 | 1. Set up [Gerrit access](gerrit_guide.md). |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 90 | 2. Tell git about your name, email and some other settings. |
| 91 | ``` |
| 92 | git config --global user.name "My Name" |
| 93 | git config --global user.email "[email protected]" |
| 94 | git config --global core.autocrlf false |
| 95 | git config --global core.filemode false |
| 96 | git config --local gerrit.host true |
| 97 | # Uncomment this if you want your pull commands to always rebase. |
| 98 | # git config --global branch.autosetuprebase always |
| 99 | # Uncomment if you want new branches to track the current branch. |
| 100 | # git config --global branch.autosetupmerge always |
| 101 | ``` |
Francois Marier | 197916f | 2020-01-16 02:23:02 | [diff] [blame] | 102 | 3. Visit <https://chromium-review.googlesource.com/settings/> to ensure that |
| 103 | your preferred email is set to the same one you use in your git |
| 104 | configuration. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 105 | |
| 106 | ## Creating a change |
| 107 | |
| 108 | First, create a new branch for your change in git. Here, we create a branch |
Emily Stark | 84d5719 | 2021-05-14 18:58:25 | [diff] [blame] | 109 | called `mychange` (use whatever name you want here), with `origin/main` as |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 110 | the upstream branch. |
| 111 | |
| 112 | ``` |
Emily Stark | 84d5719 | 2021-05-14 18:58:25 | [diff] [blame] | 113 | git checkout -b mychange -t origin/main |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 114 | ``` |
| 115 | |
| 116 | Write and test your change. |
| 117 | |
| 118 | - Conform to the [style guide][cr-styleguide]. |
| 119 | - Include tests. |
| 120 | - Patches should be a reasonable size to review. Review time often increases |
Joshua Berenhaus | 98d2fbc | 2020-01-07 18:50:42 | [diff] [blame] | 121 | exponentially with patch size. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 122 | |
| 123 | Commit your change locally in git: |
| 124 | |
| 125 | ``` |
| 126 | git commit -a |
| 127 | ``` |
| 128 | |
| 129 | If you are not familiar with `git`, GitHub's [resources to learn |
| 130 | git][github-tutorial] is useful for the basics. However, keep in mind that the |
| 131 | Chromium workflow is not the same as the GitHub pull request workflow. |
| 132 | |
| 133 | ## Uploading a change for review |
| 134 | |
Peter Kasting | 60d3028 | 2024-08-23 06:22:58 | [diff] [blame] | 135 | Note: If your change is to a dependent project, see the documentation on |
| 136 | [changing dependencies](dependencies.md#changing-dependencies). Otherwise, go |
| 137 | through the [commit checklist][commit-checklist] for Chromium before uploading a |
| 138 | change for review. |
Toby Huang | 5105f81 | 2019-08-08 23:47:57 | [diff] [blame] | 139 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 140 | Chromium uses a Gerrit instance hosted at |
| 141 | <https://chromium-review.googlesource.com> for code reviews. In order to upload |
| 142 | your local change to Gerrit, use `git-cl` from |
| 143 | [depot\_tools][depot-tools-setup] to create a new Gerrit change, based on the |
| 144 | diff between the current branch and its upstream branch: |
| 145 | |
| 146 | ``` |
| 147 | git cl upload |
| 148 | ``` |
| 149 | |
| 150 | This will open a text editor to create a description for the new change. This |
| 151 | description will be used as the commit message when the change is landed in the |
| 152 | Chromium tree. Descriptions should be formatted as follows: |
| 153 | |
| 154 | ``` |
| 155 | Summary of change (one line) |
| 156 | |
| 157 | Longer description of change addressing as appropriate: why the change |
| 158 | is made, context if it is part of many changes, description of previous |
| 159 | behavior and newly introduced differences, etc. |
| 160 | |
| 161 | Long lines should be wrapped to 72 columns for easier log message |
| 162 | viewing in terminals. |
| 163 | |
| 164 | Bug: 123456 |
| 165 | ``` |
| 166 | |
| 167 | A short subject and a blank line after the subject are crucial: `git` uses this |
| 168 | as a heuristic for tools like `git log --oneline`. Use the bug number from the |
Kalvin Lee | 2359883 | 2020-07-22 07:36:28 | [diff] [blame] | 169 | [issue tracker][crbug] (see more on [CL footer syntax](#cl-footer-reference)). |
| 170 | Also see [How to Write a Git Commit Message][good-git-commit-message], which |
| 171 | has more in-depth tips for writing a good commit description. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 172 | |
| 173 | ### Chromium-specific description tips |
| 174 | |
| 175 | - Links to previous CLs should be formatted as `https://crrev.com/c/NUMBER`, |
Kalvin Lee | 313a7f2 | 2022-08-22 08:20:45 | [diff] [blame] | 176 | which is slightly shorter than <https://chromium-review.googlesource.com>. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 177 | |
| 178 | - If there are instructions for testers to verify the change is correct, |
| 179 | include them with the `Test:` tag: |
| 180 | |
| 181 | ``` |
| 182 | Test: Load example.com/page.html and click the foo-button; see |
| 183 | crbug.com/123456 for more details. |
| 184 | ``` |
| 185 | |
| 186 | After saving the change description, `git-cl` runs some presubmit scripts to |
| 187 | check for common errors. If everything passes, `git-cl` will print something |
| 188 | like this: |
| 189 | |
| 190 | ``` |
| 191 | remote: SUCCESS |
| 192 | remote: |
| 193 | remote: New Changes: |
| 194 | remote: https://chromium-review.googlesource.com/c/chromium/src/+/1485699 Use base::TimeDelta::FromTimeSpec helper in more places. [WIP] |
| 195 | ``` |
| 196 | |
| 197 | Additional flags can be used to specify reviewers, bugs fixed by the change, et |
| 198 | cetera: |
| 199 | |
| 200 | ``` |
| 201 | git cl upload -r [email protected],[email protected] -b 123456 |
| 202 | ``` |
| 203 | |
| 204 | See `git cl help upload` for a full list of flags. |
| 205 | |
Andrea Orru | 77125524 | 2023-02-27 02:41:28 | [diff] [blame] | 206 | ### Uploading dependent changes |
| 207 | |
| 208 | If you wish to work on multiple related changes without waiting for |
| 209 | them to land, you can do so in Gerrit using dependent changes. |
| 210 | |
| 211 | To put this into an example, let‘s say you have a commit for feature A |
| 212 | and this is in the process of being reviewed on Gerrit. Now let’s say |
| 213 | you want to start more work based on it before it lands on main. |
| 214 | |
Svend Larsen | e8246932 | 2025-01-24 16:47:06 | [diff] [blame] | 215 | Gerrit has the concept of a “relation chain”. For our example above, we want to |
| 216 | create a relation chain where the feature A change is the parent of the feature |
| 217 | B change. Once we create the relation chain, the Gerrit review view for change B |
| 218 | will show the diff vs. change A, rather than vs. the main branch. |
| 219 | |
| 220 | To create a relation chain, upload a change that has an upstream branch |
| 221 | associated with the parent CL. The steps to do so are slightly different when |
| 222 | you own the parent change vs. when someone else owns the parent change. |
| 223 | |
| 224 | #### When you own the parent change |
| 225 | |
| 226 | Using the example of features A and B above, if you own the change for feature A |
| 227 | and you want to upload a dependent change for feature B: |
| 228 | |
Andrea Orru | 77125524 | 2023-02-27 02:41:28 | [diff] [blame] | 229 | ``` |
| 230 | git checkout featureA |
| 231 | git checkout -b featureB |
| 232 | git branch --set-upstream-to featureA |
| 233 | # ... edit some files |
| 234 | # ... git add ... |
| 235 | git commit |
| 236 | git cl upload |
| 237 | ``` |
| 238 | |
Svend Larsen | e8246932 | 2025-01-24 16:47:06 | [diff] [blame] | 239 | #### When someone else owns the parent change |
| 240 | |
| 241 | If someone else owns the change for feature A: |
| 242 | |
| 243 | ``` |
| 244 | # First, open the change for feature A in Gerrit -> "Download patch" -> copy and |
| 245 | # run "Branch" command. Then, starting from the branch that command created: |
| 246 | git cl patch --force <parent-CL-number> |
| 247 | git checkout -b featureB |
| 248 | git branch --set-upstream-to change-<parent-CL-number> |
| 249 | # ... edit some files |
| 250 | # ... git add ... |
| 251 | git commit |
| 252 | git cl upload |
| 253 | ``` |
| 254 | |
| 255 | Note that, after running `git cl patch --force <parent-CL-number>`, if you |
| 256 | upload changes from branch `change-<parent-CL-number>`, you will upload a new |
| 257 | patchset to the original author's change. |
| 258 | |
| 259 | If the other author uploads a new patchset to the change your change depends on, |
| 260 | you can update your local copy of their change by running: |
| 261 | |
| 262 | ``` |
| 263 | git checkout change-<parent-CL-number> |
| 264 | git fetch origin [sha hash for latest patchset] && git reset --hard FETCH_HEAD |
| 265 | ``` |
Andrea Orru | 77125524 | 2023-02-27 02:41:28 | [diff] [blame] | 266 | |
Vincent Scheib | 66fc2d4 | 2024-10-30 02:55:03 | [diff] [blame] | 267 | ## Code review {#code-review} |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 268 | |
Vincent Scheib | 66fc2d4 | 2024-10-30 02:55:03 | [diff] [blame] | 269 | This section describes the mechanics and process of code reviews. See also: |
| 270 | - [Code review policies](code_reviews.md) page for committer, OWNERS, and other |
| 271 | rules |
| 272 | - [Code of conduct](../CODE_OF_CONDUCT.md) |
| 273 | - [Respectful Changes](cl_respect.md) |
| 274 | - [Respectful Code Reviews](cr_respect.md) |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 275 | |
| 276 | ### Finding a reviewer |
| 277 | |
Ramzi N | be25013d | 2023-11-02 00:47:53 | [diff] [blame] | 278 | Please note here that a "reviewer" in this context is someone that not |
| 279 | only provides comment on the CL but also someone who can approve the |
Erik Staab | 2e34edb | 2024-02-23 18:39:40 | [diff] [blame] | 280 | submission by providing a "Code-Review +1". |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 281 | |
Ramzi N | be25013d | 2023-11-02 00:47:53 | [diff] [blame] | 282 | Reviewers must be [committers](https://www.chromium.org/getting-involved/become-a-committer/). |
| 283 | Ideally they should be committers who are familiar with the area of code |
| 284 | in question. If you're not sure who these should be, check with anyone in |
| 285 | the nearest ancestor OWNERS file. |
| 286 | |
| 287 | - There must be at least one owner for each affected directory. |
| 288 | - If there are multiple reviewers, make it clear what each reviewer is |
| 289 | expected to review. |
| 290 | - `git cl owners` automatically suggests reviewers based on the OWNERS |
| 291 | files. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 292 | |
Devlin Cronin | efe2e587 | 2020-05-06 16:34:57 | [diff] [blame] | 293 | _Note:_ By default, please only select one reviewer for each file (that is, a |
| 294 | single reviewer may review multiple files, but typically each file only needs |
| 295 | to be reviewed by one person). It can be tempting to add multiple reviewers so |
| 296 | that "whoever gets to it first" can review, but this has two common failure |
| 297 | modes: |
| 298 | - Reviewer Alpha and Beta both review the CL, resulting in duplicate effort. |
| 299 | - Out of fear of the above failure case, neither reviewer Alpha nor Beta review |
| 300 | the CL. |
| 301 | |
| 302 | There are times when requesting multiple reviewers for the same file may be |
| 303 | desirable - such as when the code is particularly complicated, or when the file |
| 304 | uses multiple systems and a perspective from each is valuable. In this case, |
| 305 | please make it explicit that you would like both reviewers to review. |
| 306 | |
Ramzi N | be25013d | 2023-11-02 00:47:53 | [diff] [blame] | 307 | Submissions to the chromium/src repository by a change contributor who is |
Erik Staab | 2e34edb | 2024-02-23 18:39:40 | [diff] [blame] | 308 | not a Chromium committer will require two committers to "Code-Review +1" the |
Ramzi N | be25013d | 2023-11-02 00:47:53 | [diff] [blame] | 309 | submissions. If the owner of the CL is already a committer, then only one |
Erik Staab | 2e34edb | 2024-02-23 18:39:40 | [diff] [blame] | 310 | other committer is needed to "Code-Review +1". |
Ramzi N | be25013d | 2023-11-02 00:47:53 | [diff] [blame] | 311 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 312 | ### Requesting review |
| 313 | |
| 314 | Open the change on [the web][crrev]. If you can't find the link, running `git |
| 315 | cl issue` will display the review URL for the current branch. Alternatively, |
| 316 | visit <https://chromium-review.googlesource.com> and look in the "Outgoing |
| 317 | Reviews" section. |
| 318 | |
| 319 | Reviewers expect to review code that compiles and passes tests. If you have |
| 320 | access, now is a good time to run your change through the [automated |
| 321 | tests](#running-automated-tests). |
| 322 | |
Avi Drissman | d8dac695 | 2024-12-04 15:49:34 | [diff] [blame] | 323 | Click the big blue **Start Review** button near the top of the page. (If you are |
| 324 | not signed in, the button will instead say "Sign In", so click it to sign in.) |
| 325 | An in-page dialog will appear, and there will be a **Reviewers** field in which |
| 326 | you can specify reviewers for the change. To the right of the field, in the |
| 327 | upper-right of the in-page dialog, will be a link titled "Suggest Owners" which, |
| 328 | when clicked, will suggest owners relevant to your change. Unless you have a |
| 329 | specific reason not to, it is recommended to click it and rely on its suggestion |
| 330 | of owners. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 331 | |
| 332 | In the same dialog, you can include an optional message to your reviewers. This |
Avi Drissman | d8dac695 | 2024-12-04 15:49:34 | [diff] [blame] | 333 | space can be used for specific questions or instructions. Once you're done, make |
| 334 | sure to click **Send and Start Review**, which notifies the requested reviewers |
| 335 | that they should review your change. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 336 | |
Avi Drissman | d8dac695 | 2024-12-04 15:49:34 | [diff] [blame] | 337 | **⚠️ Be sure to click the "Send and Start Review" button as NO ONE WILL LOOK AT |
| 338 | YOUR CHANGE UNTIL YOU DO SO ⚠️** |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 339 | |
| 340 | ### Review process |
| 341 | |
| 342 | All changes must be reviewed (see [code review policies][code-reviews]). |
| 343 | |
| 344 | You should get a response within **one** business day; re-ping your reviewers |
| 345 | if you do not. |
| 346 | |
| 347 | To upload new patch sets that address comments from the reviewers, simply |
| 348 | commit more changes to your local branch and run `git cl upload` again. |
| 349 | |
| 350 | ### Approval |
| 351 | |
| 352 | When the reviewer is happy with the change, they will set the "Code-Review +1" |
| 353 | label. Owners of all affected files must approve before a change can be |
| 354 | committed. See: [code review policies: owners][code-reviews-owners]. |
| 355 | |
Erik Staab | 3d5a4b99 | 2024-03-20 16:33:57 | [diff] [blame] | 356 | All code review comments must be marked resolved before a CL can be committed. |
| 357 | In some cases a reviewer may give "Code-Review +1" with some additional |
| 358 | comments. These should be addressed and responded to, or at least acknowledged |
| 359 | with the ACK button to resolve them. If you cannot resolve all comments an |
| 360 | override is provided through an "Unresolved-Comment-Reason:" stanza in your |
| 361 | commit message. |
| 362 | |
Devlin Cronin | 6a48482 | 2025-02-05 16:41:14 | [diff] [blame] | 363 | ### Code Review Votes |
| 364 | |
| 365 | Submitting a CL requires different approvals depending on whether the author |
| 366 | [is a committer][becoming-a-committer] and on the affected files. |
| 367 | |
| 368 | There are two types of approvals: |
| 369 | * Code-Review approvals. These are CL-wide and can be granted by any committer |
| 370 | (other than the author themselves). Committers need one Code-Review +1 |
| 371 | approval; non-committers require two separate Code-Review +1 approvals. |
| 372 | * Code-Owners approvals. Files have different owners, which are specified in the |
| 373 | `OWNERS` files. Every file requires either an owner to +1 the CL or for the |
| 374 | author to be an OWNER of the file. |
| 375 | |
| 376 | #### Copying Votes to New Patch Sets |
| 377 | |
| 378 | When a new patch set is uploaded, approvals may be removed (in order to prevent |
| 379 | someone from landing significantly different unreviewed code after getting |
| 380 | approval in a previous patch set). |
| 381 | |
| 382 | Approvals may be copied between patch sets in some situations. |
| 383 | * Code-Review approvals will be copied between patch sets if: |
| 384 | * It is a trivial rebase (as detected by Gerrit), |
| 385 | * It is a commit message change, or |
| 386 | * The author is a committer *and* the list of modified files has not changed |
| 387 | (for non-committers, any code change will result in loss of Code-Review |
| 388 | approvals). |
| 389 | * Code-Owners approvals are *always* copied between patch sets (even if the |
| 390 | reviewer's +1 is no longer shown in Gerrit). |
| 391 | |
| 392 | If a patch set loses its approvals, these will need to be re-added before the |
Devlin Cronin | 672902a | 2025-02-11 20:37:26 | [diff] [blame] | 393 | patch set can be committed (one +1 for committers, two +1s for non-committers). |
Devlin Cronin | 6a48482 | 2025-02-05 16:41:14 | [diff] [blame] | 394 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 395 | ## Running automated tests |
| 396 | |
| 397 | Before being submitted, a change must pass the commit queue (CQ). The commit |
| 398 | queue is an automated system which sends a patch to multiple try bots running |
| 399 | different platforms: each try bot compiles Chromium with the patch and ensures |
| 400 | the tests still pass on that platform. |
| 401 | |
| 402 | To trigger this process, click **CQ Dry Run** in the upper right corner of the |
| 403 | code review tool. Note that this is equivalent to setting the "Commit-Queue +1" |
| 404 | label. Anyone can set this label; however, the CQ will not process the patch |
| 405 | unless the person setting the label has [try job access][try-job-access]. |
| 406 | |
| 407 | If you don't have try job access and: |
| 408 | |
| 409 | - you have an @chromium.org email address, request access for yourself. |
| 410 | - you have contributed a few patches, ask a reviewer to nominate you for access. |
| 411 | - neither of the above is true, request that a reviewer run try jobs for you in |
| 412 | the code review request message. |
| 413 | |
| 414 | The status of the latest try job for a given patchset is visible just below the |
| 415 | list of changed files. Each bot has its own bubble, using one of the following |
| 416 | colors to indicate its status: |
| 417 | |
| 418 | - Gray: the bot has not started processing the patch yet. |
| 419 | - Yellow: the run is in progress. Check back later! |
| 420 | - Purple: the trybot encountered an exception while processing the patch. |
| 421 | Usually, this is not the fault of the patch. Try clicking **CQ Dry Run** |
| 422 | again. |
| 423 | - Red: tests failed. Click on the failed bot to see what tests failed and why. |
| 424 | - Green: the run passed! |
| 425 | |
| 426 | ## Committing |
| 427 | |
Erik Staab | 2e34edb | 2024-02-23 18:39:40 | [diff] [blame] | 428 | Changes are committed via the [commit queue][commit-queue]. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 429 | This is done by clicking **Submit to CQ** in the upper right corner, or setting |
| 430 | the "Commit-Queue +2" label on the change. The commit queue will then |
| 431 | send the patch to the try bots. If all try bots return green, the change will |
| 432 | automatically be committed. Yay! |
| 433 | |
| 434 | Sometimes a test might be flaky. If you have an isolated failure that appears |
| 435 | unrelated to your change, try sending the change to the commit queue again. |
| 436 | |
Erik Staab | 2e34edb | 2024-02-23 18:39:40 | [diff] [blame] | 437 | In emergencies, a developer with commit access can [directly |
| 438 | commit][direct-commit] a change, bypassing the commit queue and all safety nets. |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 439 | |
Ben Pastene | 893979e | 2022-10-06 17:22:55 | [diff] [blame] | 440 | ## Relanding a change |
| 441 | |
| 442 | Occasionally changes that pass the [commit queue][commit-queue] and get |
| 443 | submitted into Chromium will later be reverted. If this happens to your change, |
| 444 | don't be discouraged! This can be a common part of the Chromium development |
| 445 | cycle and happens for a variety of reasons, including a conflict with an |
| 446 | unanticipated change or tests not covered on the commit queue. |
| 447 | |
| 448 | If this happens to your change, you're encouraged to pursue a reland. When doing |
| 449 | so, following these basic steps can streamline the re-review process: |
| 450 | - **Create the reland**: Click the `CREATE RELAND` button on the original change |
| 451 | in Gerrit. This will create a new change whose diff is identical to the |
| 452 | original, but has a small paper-trail in the commit message that leads back to |
| 453 | the original. This can be useful for sheriffs when debugging regressions. |
| 454 | - **Append the fix**: If the reland requires file modifications not present in |
| 455 | the original change, simply upload these fixes in a subsequent patchset to the |
| 456 | reland change. By comparing the first patchset with the latest, this gives |
| 457 | reviewers the ability to see the diff of _just_ the reland fix. |
| 458 | - **Describe the fix**: In the commit message of the reland change, briefly |
| 459 | summarize what's changed that makes relanding again safe. Explanations can |
| 460 | include: "included needed fix", "disabled failing tests", "crash was fixed |
| 461 | elsewhere". Specifically for that last case: if the reland change is identical |
| 462 | to the original and the reland fix was handled separately in a preceding |
| 463 | change, make sure to link to that change in the commit message of the reland. |
| 464 | |
Darin Fisher | 0e196ee8 | 2019-09-06 22:39:12 | [diff] [blame] | 465 | ## Code guidelines |
| 466 | |
| 467 | In addition to the adhering to the [styleguide][cr-styleguide], the following |
| 468 | general rules of thumb can be helpful in navigating how to structure changes: |
| 469 | |
Darin Fisher | f061fb1 | 2019-11-15 23:46:13 | [diff] [blame] | 470 | - **Code in the Chromium project should be in service of other code in the |
| 471 | Chromium project.** This is important so developers can understand the |
| 472 | constraints informing a design decision. Those constraints should be apparent |
| 473 | from the scope of code within the boundary of the project and its various |
Peter Kasting | 5427510 | 2022-06-16 21:00:17 | [diff] [blame] | 474 | repositories. In general, for each line of code, you should be able to find a |
| 475 | product in the Chromium repositories that depends on that line of code or else |
| 476 | the line of code should be removed. |
Darin Fisher | 0e196ee8 | 2019-09-06 22:39:12 | [diff] [blame] | 477 | |
Peter Kasting | beb265c | 2024-10-31 20:22:56 | [diff] [blame] | 478 | When you are adding support for a new OS, architecture, compiler/STL |
| 479 | implementation, platform, or simply a new top-level directory, please send an |
| 480 | email to [email protected] and get approval. For long-term maintenance |
Kentaro Hara | c196ba1 | 2022-09-26 07:57:33 | [diff] [blame] | 481 | reasons, we will accept only things that are used by the Chromium project |
| 482 | (including Chromium-supported projects like V8 and Skia) and things whose |
| 483 | benefit to Chromium outweighs any cost increase in maintaining Chromium's |
Peter Kasting | beb265c | 2024-10-31 20:22:56 | [diff] [blame] | 484 | supported toolchains, architectures, and platforms (e.g. adding one ifdef |
| 485 | branch for an unsupported architecture has negligible cost and is likely fine, |
Kentaro Hara | c196ba1 | 2022-09-26 07:57:33 | [diff] [blame] | 486 | but introducing new abstractions or changes to higher level directories has |
| 487 | a high cost and would need to provide Chromium with corresponding benefit). |
Peter Kasting | beb265c | 2024-10-31 20:22:56 | [diff] [blame] | 488 | See the [documentation on toolchain support](toolchain_support.md) for more |
| 489 | details. Note that an unsupported configuration will not have bots on |
Kentaro Hara | c196ba1 | 2022-09-26 07:57:33 | [diff] [blame] | 490 | Google-managed waterfalls (even FYI bots) or maintained by Chromium |
| 491 | developers. Please use existing ifdef branches as much as possible. |
Dirk Pranke | bfd0b6e | 2020-06-16 23:09:53 | [diff] [blame] | 492 | |
Darin Fisher | 0e196ee8 | 2019-09-06 22:39:12 | [diff] [blame] | 493 | - **Code should only be moved to a central location (e.g., //base) when |
| 494 | multiple consumers would benefit.** We should resist the temptation to |
| 495 | build overly generic common libraries as that can lead to code bloat and |
| 496 | unnecessary complexity in common code. |
| 497 | |
| 498 | - **The code likely wasn't designed for everything we are trying to do with it |
| 499 | now.** Take time to refactor existing code to make sure the new feature or |
| 500 | subcomponent you are developing fits properly within the system. Technical |
| 501 | debt is easy to accumulate and is everyone's responsibility to avoid. |
| 502 | |
| 503 | - **Common code is everyone's responsibility.** Large files that are at the |
| 504 | cross-roads of many subsystems, where integration happens, can be some of the |
| 505 | most fragile in the system. As a companion to the previous point, be |
| 506 | cognizant of how you may be adding more complexity to the commons as you |
| 507 | venture to complete your task. |
| 508 | |
| 509 | - **Changes should include corresponding tests.** Automated testing is at the |
| 510 | heart of how we move forward as a project. All changes should include |
| 511 | corresponding tests so we can ensure that there is good coverage for code and |
| 512 | that future changes will be less likely to regress functionality. Protect |
| 513 | your code with tests! |
| 514 | |
Darin Fisher | 943fdf99 | 2020-10-29 18:02:50 | [diff] [blame] | 515 | - **Stick to the current set of supported languages as described in the |
| 516 | [styleguide][cr-styleguide].** While there is likely always a slightly better |
| 517 | tool for any particular job, maintainability of the codebase is paramount. |
| 518 | Reducing the number of languages eases toolchain and infrastructure |
| 519 | requirements, and minimizes the learning hurdles for developers to be |
| 520 | successful contributing across the codebase. Additions of new languages must |
Takuto Ikuta | 9bc7d4ef | 2023-01-06 17:55:45 | [diff] [blame] | 521 | be approved by [//ATL_OWNERS](../ATL_OWNERS). |
Darin Fisher | 943fdf99 | 2020-10-29 18:02:50 | [diff] [blame] | 522 | |
Kentaro Hara | dd8f7d70 | 2022-05-18 15:45:51 | [diff] [blame] | 523 | - **When your team is making API changes or migrating between services, the |
| 524 | team mandating the change needs to do at least 80% of the work.** The |
| 525 | rationale is to reduce externalities by having the team that requires a |
| 526 | change spend the vast majority of the time required to make it happen. |
| 527 | This naturally encourages designing to minimize the cost of change, be it |
| 528 | through automation, tooling, or pooled centralized expertise. You can find |
| 529 | more detailed rationale in [this doc](https://docs.google.com/document/d/1elJisUpOb3h4-7WA4Wn754nzfgeCJ4v2kAFvMOzNfek/edit#) |
| 530 | (Google internal). If you need an exception or help, please contact |
| 531 | [email protected]. |
| 532 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 533 | ## Tips |
| 534 | |
Dominik Röttsches | d113bfa | 2019-07-10 08:56:24 | [diff] [blame] | 535 | ### Review etiquette |
| 536 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 537 | During the lifetime of a review, you may want to rebase your change onto a newer |
| 538 | source revision to minimize merge conflicts. The reviewer-friendly way to do |
| 539 | this is to first address any unresolved comments and upload those changes as a |
| 540 | patchset. Then, rebase to the newer revision and upload that as its own |
| 541 | patchset (with no other changes). This makes it easy for reviewers to see the |
| 542 | changes made in response to their comments, and then quickly verify the diffs |
| 543 | from the rebase. |
| 544 | |
| 545 | Code authors and reviewers should keep in mind that Chromium is a global |
| 546 | project: contributors and reviewers are often in time zones far apart. Please |
| 547 | read these guidelines on [minimizing review lag][review-lag] and take them in |
| 548 | consideration both when writing reviews and responding to review feedback. |
| 549 | |
Dominik Röttsches | d113bfa | 2019-07-10 08:56:24 | [diff] [blame] | 550 | ### Watchlists |
| 551 | |
| 552 | If you would like to be notified about changes to a set of files covering a |
| 553 | topic or an area of Chromium, you may use the [watchlists][watchlist-doc] |
| 554 | feature in order to receive email notifications. |
| 555 | |
Kalvin Lee | 2359883 | 2020-07-22 07:36:28 | [diff] [blame] | 556 | ## Appendix: CL footer reference {#cl-footer-reference} |
| 557 | |
| 558 | Chromium stores a lot of information in footers at the bottom of commit |
| 559 | messages. With the exception of `R=`, these footers are only valid in the |
| 560 | last paragraph of a commit message; any footers separated from the last |
| 561 | line of the message by whitespace or non-footer lines will be ignored. |
| 562 | This includes everything from the unique `Change-Id` which identifies a |
| 563 | Gerrit change, to more useful metadata like bugs the change helps fix, |
| 564 | trybots which should be run to test the change, and more. This section |
| 565 | includes a listing of well-known footers, their meanings, and their |
| 566 | formats. |
| 567 | |
| 568 | * **Bug:** |
| 569 | * A comma-separated list of bug references. |
| 570 | * A bug reference |
| 571 | * can be a bare number, e.g. `Bug: 123456`, or |
| 572 | * can specify a project and a number, e.g. `Bug: skia:1234`. |
| 573 | * On chromium-review, the default project is assumed to be `chromium`, |
| 574 | so all bugs in non-chromium projects on bugs.chromium.org should be |
| 575 | qualified by their project name. |
| 576 | * The Google-internal issue tracker is accessible by using the `b:` |
| 577 | project prefix. |
| 578 | * **Fixed:** The same as `Bug:`, but will automatically close the |
| 579 | bug(s) as fixed when the CL lands. |
| 580 | * **R=** |
| 581 | * This footer is _deprecated_ in the Chromium project; it was |
| 582 | deprecated when code review migrated to Gerrit. Instead, use |
| 583 | `-r [email protected]` when running `git cl upload`. |
| 584 | * A comma-separated list of reviewer email addresses (e.g. |
| 585 | [email protected], [email protected]). |
Kalvin Lee | 2359883 | 2020-07-22 07:36:28 | [diff] [blame] | 586 | * **Cq-Include-Trybots:** |
| 587 | * A comma-separated list of trybots which should be triggered and |
| 588 | checked by the CQ in addition to the normal set. |
L. David Baron | 08adb30 | 2021-12-13 14:23:43 | [diff] [blame] | 589 | * Trybots are indicated in `bucket:builder` format (e.g. |
| 590 | `luci.chromium.try:android-asan`). |
| 591 | * The "Choose Tryjobs" UI in the "Checks" tab in Gerrit shows (and has |
| 592 | a button to copy) the Cq-Include-Trybots syntax for the currently |
| 593 | selected tryjobs. |
Kalvin Lee | 2359883 | 2020-07-22 07:36:28 | [diff] [blame] | 594 | * **No-Presubmit:** |
| 595 | * If present, the value should always be the string `true`. |
| 596 | * Indicates to the CQ that it should not run presubmit checks on the CL. |
| 597 | * Used primarily on automated reverts. |
| 598 | * **No-Try:** |
| 599 | * If present, the value should always be the string `true`. |
| 600 | * Indicates to the CQ that it should not start or check the results of |
| 601 | any tryjobs. |
| 602 | * Used primarily on automated reverts. |
| 603 | * **No-Tree-Checks:** |
| 604 | * If present, the value should always be the string `true`. |
| 605 | * Indicates to the CQ that it should ignore the tree status and submit |
| 606 | the change even to a closed tree. |
| 607 | * Used primarily on automated reverts. |
| 608 | * **Test:** |
| 609 | * A freeform description of manual testing performed on the change. |
| 610 | * Not necessary if all testing is covered by trybots. |
| 611 | * **Reviewed-by:** |
| 612 | * Automatically added by Gerrit when a change is submitted. |
| 613 | * Lists the names and email addresses of the people who approved |
| 614 | (set the `Code-Review` label on) the change prior to submission. |
| 615 | * **Reviewed-on:** |
| 616 | * Automatically added by Gerrit when a change is submitted. |
| 617 | * Links back to the code review page for easy access to comment and |
| 618 | patch set history. |
| 619 | * **Change-Id:** |
| 620 | * Automatically added by `git cl upload`. |
| 621 | * A unique ID that helps Gerrit keep track of commits that are part of |
| 622 | the same code review. |
| 623 | * **Cr-Commit-Position:** |
| 624 | * Automatically added by the git-numberer Gerrit plugin when a change |
| 625 | is submitted. |
| 626 | * This is of the format `fully/qualified/ref@{#123456}` and gives both |
| 627 | the branch name and "sequence number" along that branch. |
| 628 | * This approximates an SVN-style monotonically increasing revision |
| 629 | number. |
| 630 | * **Cr-Branched-From:** |
| 631 | * Automatically added by the git-numberer Gerrit plugin on changes |
Emily Stark | 84d5719 | 2021-05-14 18:58:25 | [diff] [blame] | 632 | which are submitted to non-main branches. |
| 633 | * Aids those reading a non-main branch history in finding when a |
| 634 | given commit diverged from main. |
Dominik Röttsches | d113bfa | 2019-07-10 08:56:24 | [diff] [blame] | 635 | |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 636 | [//]: # (the reference link section should be alphabetically sorted) |
Devlin Cronin | 6a48482 | 2025-02-05 16:41:14 | [diff] [blame] | 637 | [becoming-a-committer]: https://www.chromium.org/getting-involved/become-a-committer/ |
John Palmer | 046f987 | 2021-05-24 01:24:56 | [diff] [blame] | 638 | [checkout-and-build]: https://chromium.googlesource.com/chromium/src/+/main/docs/#checking-out-and-building |
John Abd-El-Malek | 27e1cf0 | 2019-12-18 17:35:18 | [diff] [blame] | 639 | [chrome-dd-review-process]: http://go/chrome-dd-review-process |
| 640 | [chromium-design-docs]: https://groups.google.com/a/chromium.org/forum/#!forum/chromium-design-docs |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 641 | [code-reviews-owners]: code_reviews.md#OWNERS-files |
| 642 | [code-reviews]: code_reviews.md |
Toby Huang | 5105f81 | 2019-08-08 23:47:57 | [diff] [blame] | 643 | [commit-checklist]: commit_checklist.md |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 644 | [commit-queue]: infra/cq.md |
| 645 | [core-principles]: https://www.chromium.org/developers/core-principles |
| 646 | [corporate-cla]: https://cla.developers.google.com/about/google-corporate?csw=1 |
| 647 | [cr-authors]: https://chromium.googlesource.com/chromium/src/+/HEAD/AUTHORS |
John Palmer | 046f987 | 2021-05-24 01:24:56 | [diff] [blame] | 648 | [cr-styleguide]: https://chromium.googlesource.com/chromium/src/+/main/styleguide/styleguide.md |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 649 | [crbug-new]: https://bugs.chromium.org/p/chromium/issues/entry |
| 650 | [crbug]: https://bugs.chromium.org/p/chromium/issues/list |
John Palmer | 046f987 | 2021-05-24 01:24:56 | [diff] [blame] | 651 | [cros-authors]: https://chromium.googlesource.com/chromium/src/+/main/AUTHORS |
| 652 | [cros-dev-guide]: https://chromium.googlesource.com/chromiumos/docs/+/main/developer_guide.md |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 653 | [crrev]: https://chromium-review.googlesource.com |
| 654 | [depot-tools-setup]: https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html#_setting_up |
John Abd-El-Malek | 27e1cf0 | 2019-12-18 17:35:18 | [diff] [blame] | 655 | [design-doc-template]: https://docs.google.com/document/d/14YBYKgk-uSfjfwpKFlp_omgUq5hwMVazy_M965s_1KA |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 656 | [direct-commit]: https://dev.chromium.org/developers/contributing-code/direct-commit |
| 657 | [discussion-groups]: https://www.chromium.org/developers/discussion-groups |
| 658 | [github-tutorial]: https://try.github.io |
| 659 | [good-git-commit-message]: https://chris.beams.io/posts/git-commit/ |
| 660 | [individual-cla]: https://cla.developers.google.com/about/google-individual?csw=1 |
Daniel Cheng | 737635ba | 2021-11-05 15:25:46 | [diff] [blame] | 661 | [life-of-a-chromium-developer]: https://docs.google.com/presentation/d/1abnqM9j6zFodPHA38JG1061rG2iGj_GABxEDgZsdbJg/edit |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 662 | [noms-tutorial]: https://meowni.ca/posts/chromium-101 |
| 663 | [review-lag]: https://dev.chromium.org/developers/contributing-code/minimizing-review-lag-across-time-zones |
Nourhan Hasan | 571a2f2 | 2024-07-26 16:50:48 | [diff] [blame] | 664 | [skia-dev-guide]: https://skia.org/docs/dev/contrib/ |
Daniel Cheng | 86135f3 | 2019-02-27 07:10:38 | [diff] [blame] | 665 | [try-job-access]: https://www.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access |
| 666 | [v8-dev-guide]: https://v8.dev/docs |
Dominik Röttsches | d113bfa | 2019-07-10 08:56:24 | [diff] [blame] | 667 | [watchlist-doc]: infra/watchlists.md |