OLD | NEW |
(Empty) | |
| 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 |
| 6 The bigger patch-upload-and-land process is covered in more detail the |
| 7 [contributing code](https://www.chromium.org/developers/contributing-code) |
| 8 page. |
| 9 |
| 10 # Code review policies |
| 11 |
| 12 Ideally the reviewer is someone who is familiar with the area of code you are |
| 13 touching. If you have doubts, look at the git blame for the file and the OWNERS |
| 14 files (see below). |
| 15 |
| 16 Any committer can review code, but there must be at least one owner for each |
| 17 directory you are touching. If you have multiple reviewers, make it clear in |
| 18 the message you send requesting review what you expect from each reviewer. |
| 19 Otherwise people might assume their input is not required or waste time with |
| 20 redundant reviews. |
| 21 |
| 22 ## Expectations for all reviewers |
| 23 |
| 24 * Aim to provide some kind of actionable response within 24 hours of receipt |
| 25 (not counting weekends and holidays). This doesn't mean you have to have |
| 26 done a complete review, but you should be able to give some initial |
| 27 feedback, request more time, or suggest another reviewer. |
| 28 |
| 29 * It can be nice to indicate if you're away in your name in the code review |
| 30 tool. If you do this, indicate when you'll be back. |
| 31 |
| 32 * Don't generally discourage people from sending you code reviews. This |
| 33 includes writing a blanket ("slow") after your name in the review tool. |
| 34 |
| 35 ## OWNERS files |
| 36 |
| 37 In various directories there are files named "OWNERS" that list the email |
| 38 addresses of people qualified to review changes in that directory. You must |
| 39 get a positive review from an owner of each directory your change touches. |
| 40 |
| 41 Owners files are recursive, so each file also applies to its subdirectories. If |
| 42 an owner file contains the text "set noparent" it will stop owner propagation |
| 43 from parent directories. |
| 44 |
| 45 Tip: The "git cl owners" command can help find owners. |
| 46 |
| 47 While owners must approve all patches, any committer can contribute to the |
| 48 review. In some directories the owners can be overloaded or there might be |
| 49 people not listed as owners who are more familiar with the low-level code in |
| 50 question. In these cases it's common to request a low-level review from an |
| 51 appropriate person, and then request a high-level owner review once that's |
| 52 complete. As always, be clear what you expect of each reviewer to avoid |
| 53 duplicated work. |
| 54 |
| 55 ### Expectations for owners |
| 56 |
| 57 * OWNERS files should contain only people actively reviewing code in that |
| 58 directory. |
| 59 |
| 60 * If you aren't doing code reviews in a directory, remove yourself. Don't |
| 61 try to discourage people from sending reviews in comments in the OWNERS |
| 62 file. This includes writing "slow" or "emeritus" after your name. |
| 63 |
| 64 * If the code review load is unsustainable, work to expand the number of |
| 65 owners. |
| 66 |
| 67 ## TBR ("To Be Reviewed") |
| 68 |
| 69 "TBR" is our mechanism for post-commit review. It should be used rarely and |
| 70 only in cases where a review is unnecessary or as described below. The most |
| 71 common use of TBR is to revert patches that broke the build. |
| 72 |
| 73 TBR does not mean "no review." A reviewer on a TBRed on a change should still |
| 74 review the change. If there comments after landing the author is still |
| 75 obligated to address them in a followup patch. |
| 76 |
| 77 Do not use TBR just because a change is urgent or the reviewer is being slow. |
| 78 Work to contact a reviewer who can do your review in a timely manner. |
| 79 |
| 80 To send a change TBR, annotate the description and send email like normal. |
| 81 Otherwise the reviewer won't know to review the patch. |
| 82 |
| 83 * Add the reviewer's email address in the code review tool's reviewer field |
| 84 like normal. |
| 85 |
| 86 * Add a line "TBR=<reviewer's email>" to the bottom of the change list |
| 87 description. |
| 88 |
| 89 * Push the "send mail" button. |
| 90 |
| 91 ### TBR-ing certain types of mechanical changes |
| 92 |
| 93 Sometimes you might do something that affects many callers in different |
| 94 directories. For example, adding a parameter to a common function in //base. |
| 95 If the updates to the callers is mechanical, you can: |
| 96 |
| 97 * Get a normal owner of the lower-level directory (in this example, //base) |
| 98 you're changing to do a proper review of those changes. |
| 99 |
| 100 * Get _somebody_ to review the downstream changes. This is often the same |
| 101 person from the previous step but could be somebody else. |
| 102 |
| 103 * Add the owners of the affected downstream directories as TBR. |
| 104 |
| 105 This process ensures that all code is reviewed prior to checkin and that the |
| 106 concept of the change is reviewed by a qualified person, but you don't have to |
| 107 track down many individual owners for trivial changes to their directories. |
| 108 |
| 109 ### TBR-ing documentation updates |
| 110 |
| 111 You can TBR documentation updates. Documentation means markdown files, text |
| 112 documents, and high-level comments in code. At finer levels of detail, comments |
| 113 in source files become more like code and should be reviewed normally (not |
| 114 using TBR). Non-TBR-able stuff includes things like function contracts and most |
| 115 comments inside functions. |
| 116 |
| 117 * Use your best judgement. If you're changing something very important, |
| 118 tricky, or something you may not be very familiar with, ask for the code |
| 119 review up-front. |
| 120 |
| 121 * Don't TBR changes to policy documents like the style guide or this document. |
| 122 |
| 123 * Don't mix unrelated documentation updates with code changes. |
| 124 |
| 125 * Be sure to actually send out the email for the code review. If you get one, |
| 126 please actually read the changes. Even the best writers have editors and |
| 127 checking prose for clarity is much faster than checking code for |
| 128 correctness. |
OLD | NEW |