Chromium Code Reviews| OLD | NEW |
|---|---|
| (Empty) | |
| 1 How to submit a patch | |
| 2 ===================== | |
| 3 | |
| 4 | |
| 5 Making changes | |
| 6 -------------- | |
| 7 | |
| 8 First create a branch for your changes: | |
| 9 | |
| 10 ~~~~ | |
| 11 $ git checkout --track origin/master -b my_feature master | |
| 12 ~~~~ | |
| 13 | |
| 14 After making your changes, create a commit | |
| 15 | |
| 16 ~~~~ | |
| 17 $ git add [file1] [file2] ... | |
| 18 $ git commit | |
| 19 ~~~~ | |
| 20 | |
| 21 If your branch gets out of date, you will need to update it: | |
| 22 | |
| 23 ~~~~ | |
| 24 $ git pull --rebase | |
| 25 $ gclient sync | |
| 26 ~~~~ | |
| 27 | |
| 28 | |
| 29 Adding a unit test | |
| 30 ------------------ | |
| 31 | |
| 32 If you are willing to change Skia codebase, it's nice to add a test at the same | |
| 33 time. Skia has a simple unittest framework so you can add a case to it. | |
| 34 | |
| 35 Test code is located under the 'tests' directory. Assuming we are adding | |
| 36 tests/FooTest.cpp, The test code will look like: | |
| 37 | |
| 38 ~~~~ | |
|
jcgregorio
2015/01/21 17:55:39
<!--?prettify?-->
| |
| 39 /* | |
| 40 * Copyright ........ | |
| 41 * | |
| 42 * Use of this source code is governed by a BSD-style license that can be | |
| 43 * found in the LICENSE file. | |
| 44 */ | |
| 45 | |
| 46 #include "Test.h" | |
| 47 | |
| 48 DEF_TEST(TestFoo, reporter) { | |
| 49 int x = 2 * 3; | |
| 50 if (x != 6) { | |
| 51 ERRORF(reporter, "x should be 6, but is %d", x); | |
| 52 return; | |
| 53 } | |
| 54 REPORTER_ASSERT(reporter, 1 + 1 == 2); | |
| 55 } | |
| 56 ~~~~ | |
| 57 | |
| 58 And we need to add this new file to gyp/tests.gyp. Note that file names are | |
| 59 sorted alphabetically. | |
| 60 | |
| 61 ~~~~ | |
|
jcgregorio
2015/01/21 17:55:39
<!--?prettify?-->
| |
| 62 'sources': [ | |
| 63 '../tests/AAClipTest.cpp' | |
| 64 '../tests/FooTest.cpp', | |
| 65 '../tests/XfermodeTest.cpp', | |
| 66 ], | |
| 67 ~~~~ | |
| 68 | |
| 69 Unit tests are best, but if your change touches rendering and you can't think of | |
| 70 an automated way to verify the results, consider writing a GM test or a new page | |
| 71 of SampleApp. Also, if your change is the GPU code, you may not be able to write | |
| 72 it as part of the standard unit test suite, but there are GPU-specific testing | |
| 73 paths you can extend. | |
| 74 | |
| 75 | |
| 76 Submitting a patch | |
| 77 ------------------ | |
| 78 | |
| 79 For your code to be accepted into the codebase, you must complete the | |
| 80 [Individual Contributor License | |
| 81 Agreement](http://code.google.com/legal/individual-cla-v1.0.html). You can do | |
| 82 this online, and it only takes a minute. If you are contributing on behalf of a | |
| 83 corporation, you must fill out the [Corporate Contributor License Agreement](htt p://code.google.com/legal/corporate-cla-v1.0.html) | |
| 84 and send it to us as described on that page. Add your (or your organization's) | |
| 85 name and contact info to the AUTHORS file as a part of your CL. | |
| 86 | |
| 87 Now that you've made a change and written a test for it, it's ready for the code | |
| 88 review! Submit a patch and getting it reviewed is fairly easy with depot tools. | |
| 89 | |
| 90 Use git-cl, which comes with [depot tools](http://sites.google.com/a/chromium.or g/dev/developers/how-tos/install-depot-tools). | |
| 91 For help, run git-cl help. | |
| 92 | |
| 93 ### Configuring git-cl | |
| 94 | |
| 95 Before using any git-cl commands you will need to configure it to point at the | |
| 96 correct code review server. This is accomplished with the following command: | |
| 97 | |
| 98 ~~~~ | |
| 99 git cl config https://skia.googlesource.com/skia/+/master/codereview.settings | |
| 100 ~~~~ | |
| 101 | |
| 102 ### Find a reviewer | |
| 103 | |
| 104 Ideally, the reviewer is someone who is familiar with the area of code you are | |
| 105 touching. If you have doubts, look at the git blame for the file to see who else | |
| 106 has been editing it. | |
| 107 | |
| 108 ### Uploading changes for review | |
| 109 | |
| 110 Skia uses Chromium's code review [site](http://codereview.chromium.org) and the | |
| 111 Rietveld open source code review tool. | |
| 112 Use git cl to upload your change: | |
| 113 ~~~~ | |
| 114 $ git cl upload | |
| 115 ~~~~ | |
| 116 | |
| 117 You may have to enter a Google Account username and password to authenticate | |
| 118 yourself to codereview.chromium.org. A free gmail account will do fine, or any | |
| 119 other type of Google account. It does not have to match the email address you | |
| 120 configured using git config --global user.email above, but it can. | |
| 121 | |
| 122 The command output should include a URL, similar to | |
| 123 (https://codereview.chromium.org/111893004/), indicating where your changelist | |
| 124 can be reviewed. | |
| 125 | |
| 126 ### Request review | |
| 127 | |
| 128 Go to the supplied URL or go to the code review page and click **Issues created | |
| 129 by me**. Select the change you want to submit for review and click **Edit | |
| 130 Issue**. Enter at least one reviewer's email address and click **Update Issue**. | |
| 131 Now click on **Publish+Mail Comments**, add any optional notes, and send your | |
| 132 change off for review. Unless you publish your change, no one will know to look | |
| 133 at it. | |
| 134 | |
| 135 _Note_: If you don't see editing commands on the review page, click **Log In** | |
| 136 in the upper right. _Hint_: You can add -r reviewer@example.com --send-mail to | |
| 137 send the email directly when uploading a change in both gcl and git-cl. | |
| 138 | |
| 139 | |
| 140 The review process | |
| 141 ------------------ | |
| 142 | |
| 143 If you submit a giant patch, or do a bunch of work without discussing it with | |
| 144 the relevant people, you may have a hard time convincing anyone to review it! | |
| 145 | |
| 146 Please follow the guidelines on how to conduct a code review detailed here: | |
| 147 https://code.google.com/p/rietveld/wiki/CodeReviewHelp | |
| 148 | |
| 149 Code reviews are an important part of the engineering process. The reviewer will | |
| 150 almost always have suggestions or style fixes for you, and it's important not to | |
| 151 take such suggestions personally or as a commentary on your abilities or ideas. | |
| 152 This is a process where we work together to make sure that the highest quality | |
| 153 code gets submitted! | |
| 154 | |
| 155 You will likely get email back from the reviewer with comments. Fix these and | |
| 156 update the patch set in the issue by uploading again. The upload will explain | |
| 157 that it is updating the current CL and ask you for a message explaining the | |
| 158 change. Be sure to respond to all comments before you request review of an | |
| 159 update. | |
| 160 | |
| 161 If you need to update code the code on an already uploaded CL, simply edit the | |
| 162 code, commit it again locally, and then run git cl upload again e.g. | |
| 163 | |
| 164 ~~~~ | |
| 165 echo "" > GOATS | |
| 166 git add GOATS | |
| 167 git commit -m 'add newline fix to GOATS' | |
| 168 git cl upload | |
| 169 ~~~~ | |
| 170 | |
| 171 Once you're ready for another review, use **Publish+Mail Comments** again to | |
| 172 send another notification (it is helpful to tell the review what you did with | |
| 173 respect to each of their comments). When the reviewer is happy with your patch, | |
| 174 they will say "LGTM" ("Looks Good To Me"). | |
| 175 | |
| 176 _Note_: As you work through the review process, both you and your reviewers | |
| 177 should converse using the code review interface, and send notes using | |
| 178 **Publish+Mail Comments**. | |
| 179 | |
| 180 Once your commit has gone in, you should delete the branch containing your chang e: | |
| 181 | |
| 182 ~~~~ | |
| 183 $ git checkout master | |
| 184 $ git branch -D my_feature | |
| 185 ~~~~ | |
| 186 | |
| 187 | |
| 188 Final Testing | |
| 189 ------------- | |
| 190 | |
| 191 Skia's principal downstream user is Chromium, and any change to Skia rendering | |
| 192 output can break Chromium. If your change alters rendering in any way, you are | |
| 193 expected to test for and alleviate this. (You may be able to find a Skia team | |
| 194 member to help you, but the onus remains on each individual contributor to avoid | |
| 195 breaking Chrome. | |
| 196 | |
| 197 ### Evaluating Impact on Chromium | |
| 198 | |
| 199 Keep in mind that Skia is rolled daily into Blink and Chromium. Run local tests | |
| 200 and watch canary bots for results to ensure no impact. If you are submitting | |
| 201 changes that will impact layout tests, follow the guides below and/or work with | |
| 202 your friendly Skia-Blink engineer to evaluate, rebaseline, and land your | |
| 203 changes. | |
| 204 | |
| 205 Resources: | |
| 206 | |
| 207 [How to land Skia changes that change Blink layout test results](../chrome/layou ttest) | |
| 208 | |
| 209 If you're changing the Skia API, you may need to make an associated change in Ch romium. | |
| 210 If you do, please follow these instructions: [Landing Skia changes which require Chrome changes](../chrome/changes) | |
| 211 | |
| 212 | |
| 213 Check in your changes | |
| 214 --------------------- | |
| 215 | |
| 216 ### Non-Skia-committers | |
| 217 | |
| 218 If you already have committer rights, you can follow the directions below to | |
| 219 commit your change directly to Skia's repository. | |
| 220 | |
| 221 If you don't have committer rights in https://skia.googlesource.com/skia.git ... | |
| 222 first of all, thanks for submitting your patch! We really appreciate these | |
| 223 submissions. Unfortunately, we don't yet have a way for Skia committers to mark | |
| 224 a patch as "approved" and thus allow non-committers to commit them. So instead, | |
| 225 please ask a Skia committer to land your patch for you or land using the commit | |
| 226 queue. | |
| 227 | |
| 228 As part of this process, the Skia committer may create a new codereview | |
| 229 containing your patch (perhaps with some small adjustments at her discretion). | |
| 230 If so, you can mark your codereview as "Closed", and update it with a link to | |
| 231 the new codereview. | |
| 232 | |
| 233 ### Skia committers: | |
| 234 * tips on how to apply the externally provided patch are [here](./patch) | |
| 235 * when landing externally contributed patches, please note the original | |
| 236 contributor's identity (and provide a link to the original codereview) in t he commit message | |
| 237 | |
| 238 git-cl will squash all your commits into a single one with the description you u sed when you uploaded your change. | |
| 239 | |
| 240 ~~~~ | |
| 241 git cl land | |
| 242 ~~~~ | |
| 243 or | |
| 244 ~~~~ | |
| 245 git cl land -c 'Contributor Name <email@example.com>' | |
| 246 ~~~~ | |
| OLD | NEW |