Chromium Code Reviews| Index: site/dev/contrib/submit.md |
| diff --git a/site/dev/contrib/submit.md b/site/dev/contrib/submit.md |
| new file mode 100644 |
| index 0000000000000000000000000000000000000000..0ece35d2911e6c38d2b0d3c68d93017008ca5442 |
| --- /dev/null |
| +++ b/site/dev/contrib/submit.md |
| @@ -0,0 +1,246 @@ |
| +How to submit a patch |
| +===================== |
| + |
| + |
| +Making changes |
| +-------------- |
| + |
| +First create a branch for your changes: |
| + |
| +~~~~ |
| +$ git checkout --track origin/master -b my_feature master |
| +~~~~ |
| + |
| +After making your changes, create a commit |
| + |
| +~~~~ |
| +$ git add [file1] [file2] ... |
| +$ git commit |
| +~~~~ |
| + |
| +If your branch gets out of date, you will need to update it: |
| + |
| +~~~~ |
| +$ git pull --rebase |
| +$ gclient sync |
| +~~~~ |
| + |
| + |
| +Adding a unit test |
| +------------------ |
| + |
| +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. |
| + |
| +Test code is located under the 'tests' directory. Assuming we are adding |
| +tests/FooTest.cpp, The test code will look like: |
| + |
| +~~~~ |
|
jcgregorio
2015/01/21 17:55:39
<!--?prettify?-->
|
| +/* |
| + * Copyright ........ |
| + * |
| + * Use of this source code is governed by a BSD-style license that can be |
| + * found in the LICENSE file. |
| + */ |
| + |
| +#include "Test.h" |
| + |
| +DEF_TEST(TestFoo, reporter) { |
| + int x = 2 * 3; |
| + if (x != 6) { |
| + ERRORF(reporter, "x should be 6, but is %d", x); |
| + return; |
| + } |
| + REPORTER_ASSERT(reporter, 1 + 1 == 2); |
| +} |
| +~~~~ |
| + |
| +And we need to add this new file to gyp/tests.gyp. Note that file names are |
| +sorted alphabetically. |
| + |
| +~~~~ |
|
jcgregorio
2015/01/21 17:55:39
<!--?prettify?-->
|
| +'sources': [ |
| + '../tests/AAClipTest.cpp' |
| + '../tests/FooTest.cpp', |
| + '../tests/XfermodeTest.cpp', |
| +], |
| +~~~~ |
| + |
| +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 page |
| +of SampleApp. Also, if your change is the GPU code, you may not be able to write |
| +it as part of the standard unit test suite, but there are GPU-specific testing |
| +paths you can extend. |
| + |
| + |
| +Submitting a patch |
| +------------------ |
| + |
| +For your code to be accepted into the codebase, you must complete the |
| +[Individual 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 contributing on behalf of a |
| +corporation, you must fill out the [Corporate Contributor License 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 contact info to the AUTHORS file as a part of your CL. |
| + |
| +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. |
| + |
| +Use git-cl, which comes with [depot tools](http://sites.google.com/a/chromium.org/dev/developers/how-tos/install-depot-tools). |
| +For help, run git-cl help. |
| + |
| +### Configuring git-cl |
| + |
| +Before using any git-cl commands you will need to configure it to point at the |
| +correct code review server. This is accomplished with the following command: |
| + |
| +~~~~ |
| +git cl config https://skia.googlesource.com/skia/+/master/codereview.settings |
| +~~~~ |
| + |
| +### Find a reviewer |
| + |
| +Ideally, the reviewer is someone who is familiar with the area of code you are |
| +touching. If you have doubts, look at the git blame for the file to see who else |
| +has been editing it. |
| + |
| +### Uploading changes for review |
| + |
| +Skia uses Chromium's code review [site](http://codereview.chromium.org) and the |
| +Rietveld open source code review tool. |
| +Use git cl to upload your change: |
| +~~~~ |
| +$ git cl upload |
| +~~~~ |
| + |
| +You may have to enter a Google Account username and password to authenticate |
| +yourself to codereview.chromium.org. A free gmail account will do fine, or any |
| +other type of Google account. It does not have to match the email address you |
| +configured using git config --global user.email above, but it can. |
| + |
| +The command output should include a URL, similar to |
| +(https://codereview.chromium.org/111893004/), indicating where your changelist |
| +can be reviewed. |
| + |
| +### Request review |
| + |
| +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 click 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. |
| + |
| +_Note_: If you don't see editing commands on the review page, click **Log In** |
| +in the upper right. _Hint_: You can add -r reviewer@example.com --send-mail to |
| +send the email directly when uploading a change in both gcl and git-cl. |
| + |
| + |
| +The review process |
| +------------------ |
| + |
| +If you submit a giant patch, or do a bunch of work without discussing it with |
| +the relevant people, you may have a hard time convincing anyone to review it! |
| + |
| +Please follow the guidelines on how to conduct a code review detailed here: |
| +https://code.google.com/p/rietveld/wiki/CodeReviewHelp |
| + |
| +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 to |
| +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! |
| + |
| +You will likely get email back from the reviewer with comments. Fix these and |
| +update 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. |
| + |
| +If you need to update code the code on an already uploaded CL, simply edit the |
| +code, commit it again locally, and then run git cl upload again e.g. |
| + |
| +~~~~ |
| +echo "" > GOATS |
| +git add GOATS |
| +git commit -m 'add newline fix to GOATS' |
| +git cl upload |
| +~~~~ |
| + |
| +Once you're ready for another review, use **Publish+Mail Comments** again to |
| +send another notification (it is helpful to tell the review what you did with |
| +respect to each of their comments). When the reviewer is happy with your patch, |
| +they will say "LGTM" ("Looks Good To Me"). |
| + |
| +_Note_: As you work through the review process, both you and your reviewers |
| +should converse using the code review interface, and send notes using |
| +**Publish+Mail Comments**. |
| + |
| +Once your commit has gone in, you should delete the branch containing your change: |
| + |
| +~~~~ |
| +$ git checkout master |
| +$ git branch -D my_feature |
| +~~~~ |
| + |
| + |
| +Final Testing |
| +------------- |
| + |
| +Skia's principal downstream user is Chromium, and any change to Skia rendering |
| +output can break Chromium. If your change alters rendering in any way, you are |
| +expected to test for and alleviate this. (You may be able to find a Skia team |
| +member to help you, but the onus remains on each individual contributor to avoid |
| +breaking Chrome. |
| + |
| +### Evaluating Impact on Chromium |
| + |
| +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 |
| +changes that will impact layout tests, follow the guides below and/or work with |
| +your friendly Skia-Blink engineer to evaluate, rebaseline, and land your |
| +changes. |
| + |
| +Resources: |
| + |
| +[How to land Skia changes that change Blink layout test results](../chrome/layouttest) |
| + |
| +If you're changing the Skia API, you may need to make an associated change in Chromium. |
| +If you do, please follow these instructions: [Landing Skia changes which require Chrome changes](../chrome/changes) |
| + |
| + |
| +Check in your changes |
| +--------------------- |
| + |
| +### Non-Skia-committers |
| + |
| +If you already have committer rights, you can follow the directions below to |
| +commit your change directly to Skia's repository. |
| + |
| +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 |
| +submissions. 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, |
| +please ask a Skia committer to land your patch for you or land using the commit |
| +queue. |
| + |
| +As part of this process, the Skia committer may create a new codereview |
| +containing your patch (perhaps with some small adjustments at her discretion). |
| +If so, you can mark your codereview as "Closed", and update it with a link to |
| +the new codereview. |
| + |
| +### Skia committers: |
| + * tips on how to apply the externally provided patch are [here](./patch) |
| + * when landing externally contributed patches, please note the original |
| + contributor's identity (and provide a link to the original codereview) in the commit message |
| + |
| +git-cl will squash all your commits into a single one with the description you used when you uploaded your change. |
| + |
| +~~~~ |
| +git cl land |
| +~~~~ |
| +or |
| +~~~~ |
| +git cl land -c 'Contributor Name <email@example.com>' |
| +~~~~ |