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 |