Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(5)

Side by Side Diff: site/dev/contrib/submit.md

Issue 844433004: Add Contributing to Skia section of docs (Closed) Base URL: https://skia.googlesource.com/skia.git@master
Patch Set: fix code block on flatten page Created 5 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « site/dev/contrib/style.md ('k') | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
(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 ~~~~
OLDNEW
« no previous file with comments | « site/dev/contrib/style.md ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698