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

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 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/patch.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 time. Skia has a simple unittest framework so you can add a case to it.
33
34 Test code is located under the 'tests' directory. Assuming we are adding tests/F ooTest.cpp, The test code will look like:
35
36 ~~~~
37 /*
38 * Copyright ........
39 *
40 * Use of this source code is governed by a BSD-style license that can be
41 * found in the LICENSE file.
42 */
43
44 #include "Test.h"
45
46 DEF_TEST(TestFoo, reporter) {
47 int x = 2 * 3;
48 if (x != 6) {
49 ERRORF(reporter, "x should be 6, but is %d", x);
50 return;
51 }
52 REPORTER_ASSERT(reporter, 1 + 1 == 2);
53 }
54 ~~~~
55
56 And we need to add this new file to gyp/tests.gyp. Note that file names are sort ed alphabetically.
57
58 ~~~~
59 'sources': [
60 '../tests/AAClipTest.cpp'
61 '../tests/FooTest.cpp',
62 '../tests/XfermodeTest.cpp',
63 ],
64 ~~~~
65
66 Unit tests are best, but if your change touches rendering and you can't think of an automated way to verify the results, consider writing a GM test or a new pag e of SampleApp. Also, if your change is the GPU code, you may not be able to wri te it as part of the standard unit test suite, but there are GPU-specific testin g paths you can extend.
67
68
69 Submitting a patch
70 ------------------
71
72 For your code to be accepted into the codebase, you must complete the [Individua l Contributor License Agreement](http://code.google.com/legal/individual-cla-v1. 0.html). You can do this online, and it only takes a minute. If you are contribu ting on behalf of a corporation, you must fill out the [Corporate Contributor Li cense Agreement](http://code.google.com/legal/corporate-cla-v1.0.html) and send it to us as described on that page. Add your (or your organization's) name and c ontact info to the AUTHORS file as a part of your CL.
73
74 Now that you've made a change and written a test for it, it's ready for the code review! Submit a patch and getting it reviewed is fairly easy with depot tools.
75
76 Use git-cl, which comes with [depot tools](http://sites.google.com/a/chromium.or g/dev/developers/how-tos/install-depot-tools). For help, run git-cl help.
77
78 ### Configuring git-cl
79
80 Before using any git-cl commands you will need to configure it to point at the c orrect code review server. This is accomplished with the following command:
81
82 ~~~~
83 git cl config https://skia.googlesource.com/skia/+/master/codereview.settings
84 ~~~~
85
86 ### Find a reviewer
87
88 Ideally, the reviewer is someone who is familiar with the area of code you are t ouching. If you have doubts, look at the git blame for the file to see who else has been editing it.
89
90 ### Uploading changes for review
91
92 Skia uses Chromium's code review [site](http://codereview.chromium.org) and the Rietveld open source code review tool.
93 Use git cl to upload your change:
94 ~~~~
95 $ git cl upload
96 ~~~~
97
98 You may have to enter a Google Account username and password to authenticate you rself to codereview.chromium.org. A free gmail account will do fine, or any othe r type of Google account. It does not have to match the email address you confi gured using git config --global user.email above, but it can.
99
100 The command output should include a URL, similar to (https://codereview.chromium .org/111893004/), indicating where your changelist can be reviewed.
101
102 ### Request review
103
104 Go to the supplied URL or go to the code review page and click **Issues created by me**. Select the change you want to submit for review and click **Edit Issue* *. Enter at least one reviewer's email address and click **Update Issue**. Now c lick on **Publish+Mail Comments**, add any optional notes, and send your change off for review. Unless you publish your change, no one will know to look at it.
105
106 _Note_: If you don't see editing commands on the review page, click **Log In** i n the upper right. _Hint_: You can add -r reviewer@example.com --send-mail to se nd the email directly when uploading a change in both gcl and git-cl.
107
108
109 The review process
110 ------------------
111
112 If you submit a giant patch, or do a bunch of work without discussing it with th e relevant people, you may have a hard time convincing anyone to review it!
113
114 Please follow the guidelines on how to conduct a code review detailed here: http s://code.google.com/p/rietveld/wiki/CodeReviewHelp
115
116 Code reviews are an important part of the engineering process. The reviewer will almost always have suggestions or style fixes for you, and it's important not t o take such suggestions personally or as a commentary on your abilities or ideas . This is a process where we work together to make sure that the highest quality code gets submitted!
117
118 You will likely get email back from the reviewer with comments. Fix these and up date the patch set in the issue by uploading again. The upload will explain that it is updating the current CL and ask you for a message explaining the change. Be sure to respond to all comments before you request review of an update.
119
120 If you need to update code the code on an already uploaded CL, simply edit the c ode, commit it again locally, and then run git cl upload again e.g.
121
122 ~~~~
123 echo "" > GOATS
124 git add GOATS
125 git commit -m 'add newline fix to GOATS'
126 git cl upload
127 ~~~~
128
129 Once you're ready for another review, use **Publish+Mail Comments** again to sen d another notification (it is helpful to tell the review what you did with respe ct to each of their comments). When the reviewer is happy with your patch, they will say "LGTM" ("Looks Good To Me").
130
131 _Note_: As you work through the review process, both you and your reviewers shou ld converse using the code review interface, and send notes using **Publish+Mail Comments**.
132
133 Once your commit has gone in, you should delete the branch containing your chang e:
134
135 ~~~~
136 $ git checkout master
137 $ git branch -D my_feature
138 ~~~~
139
140
141 Final Testing
142 -------------
143
144 Skia's principal downstream user is Chromium, and any change to Skia rendering o utput can break Chromium. If your change alters rendering in any way, you are ex pected to test for and alleviate this. (You may be able to find a Skia team memb er to help you, but the onus remains on each individual contributor to avoid bre aking Chrome.
145
146 ### Evaluating Impact on Chromium
147
148 Keep in mind that Skia is rolled daily into Blink and Chromium. Run local tests and watch canary bots for results to ensure no impact. If you are submitting c hanges that will impact layout tests, follow the guides below and/or work with y our friendly Skia-Blink engineer to evaluate, rebaseline, and land your changes.
149
150 Resources:
151
152 [How to land Skia changes that change Blink layout test results](../chrome/layou ttest)
153
154 If you're changing the Skia API, you may need to make an associated change in Ch romium. If you do, please follow these instructions: [Landing Skia changes whic h require associated Chrome changes](../chrome/changes)
155
156
157 Check in your changes
158 ---------------------
159
160 ### Non-Skia-committers
161
162 If you already have committer rights, you can follow the directions below to com mit your change directly to Skia's repository.
163
164 If you don't have committer rights in https://skia.googlesource.com/skia.git ... first of all, thanks for submitting your patch! We really appreciate these sub missions. Unfortunately, we don't yet have a way for Skia committers to mark a patch as "approved" and thus allow non-committers to commit them. So instead, p lease ask a Skia committer to land your patch for you or land using the commit q ueue.
165
166 As part of this process, the Skia committer may create a new codereview containi ng your patch (perhaps with some small adjustments at her discretion). If so, y ou can mark your codereview as "Closed", and update it with a link to the new co dereview.
167
168 ### Skia committers:
169 * tips on how to apply the externally provided patch are [here](./patch)
170 * when landing externally contributed patches, please note the original contr ibutor's identity (and provide a link to the original codereview) in the commit message
171
172 git-cl will squash all your commits into a single one with the description you u sed when you uploaded your change.
173
174 ~~~~
175 git cl land
176 ~~~~
177 or
178 ~~~~
179 git cl land -c 'Contributor Name <email@example.com>'
180 ~~~~
OLDNEW
« site/dev/contrib/patch.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