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

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: small fix to style guide date formatting 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
« site/dev/contrib/style.md ('K') | « 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 ~~~~
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 ~~~~
OLDNEW
« site/dev/contrib/style.md ('K') | « site/dev/contrib/style.md ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698