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