Chromium Code Reviews| OLD | NEW |
|---|---|
| 1 How to submit a patch | 1 How to submit a patch |
| 2 ===================== | 2 ===================== |
| 3 | 3 |
| 4 | 4 |
| 5 Configure git | |
| 6 ------------- | |
| 7 | |
| 8 <!--?prettify lang=sh?--> | |
| 9 | |
| 10 $ git config --global user.name "Your Name" | |
| 11 $ git config --global user.email you@example.com | |
| 12 | |
| 5 Making changes | 13 Making changes |
| 6 -------------- | 14 -------------- |
| 7 | 15 |
| 8 First create a branch for your changes: | 16 First create a branch for your changes: |
| 9 | 17 |
| 10 ~~~~ | 18 $ git config branch.autosetuprebase always |
|
hcm
2015/11/09 18:23:55
As discussed, either note this as optional and/or
hal.canary
2015/11/09 19:45:50
done
| |
| 11 $ git checkout -b my_feature origin/master | 19 $ git checkout -b my_feature origin/master |
| 12 ~~~~ | |
| 13 | 20 |
| 14 After making your changes, create a commit | 21 After making your changes, create a commit |
| 15 | 22 |
| 16 ~~~~ | 23 $ git add [file1] [file2] ... |
| 17 $ git add [file1] [file2] ... | 24 $ git commit |
| 18 $ git commit | |
| 19 ~~~~ | |
| 20 | 25 |
| 21 If your branch gets out of date, you will need to update it: | 26 If your branch gets out of date, you will need to update it: |
| 22 | 27 |
| 23 ~~~~ | 28 $ git pull --rebase |
| 24 $ git pull --rebase | 29 $ python bin/sync-and-gyp |
| 25 $ gclient sync | |
| 26 ~~~~ | |
| 27 | 30 |
| 31 <!-- | |
| 32 $ python tools/git-sync-deps | |
| 33 $ python ./gyp_skia | |
| 34 --> | |
| 28 | 35 |
| 29 Adding a unit test | 36 Adding a unit test |
| 30 ------------------ | 37 ------------------ |
| 31 | 38 |
| 32 If you are willing to change Skia codebase, it's nice to add a test at the same | 39 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. | 40 time. Skia has a simple unittest framework so you can add a case to it. |
| 34 | 41 |
| 35 Test code is located under the 'tests' directory. | 42 Test code is located under the 'tests' directory. |
| 36 | 43 |
| 37 See [Writing Unit and Rendering Tests](tests) for details. | 44 See [Writing Unit and Rendering Tests](../testing/tests) for details. |
| 38 | 45 |
| 39 Unit tests are best, but if your change touches rendering and you can't think of | 46 Unit tests are best, but if your change touches rendering and you can't think of |
| 40 an automated way to verify the results, consider writing a GM test or a new page | 47 an automated way to verify the results, consider writing a GM test or a new page |
| 41 of SampleApp. Also, if your change is the GPU code, you may not be able to write | 48 of SampleApp. Also, if your change is the GPU code, you may not be able to write |
| 42 it as part of the standard unit test suite, but there are GPU-specific testing | 49 it as part of the standard unit test suite, but there are GPU-specific testing |
| 43 paths you can extend. | 50 paths you can extend. |
| 44 | 51 |
| 45 Submitting a patch | 52 Submitting a patch |
| 46 ------------------ | 53 ------------------ |
| 47 | 54 |
| 48 For your code to be accepted into the codebase, you must complete the | 55 For your code to be accepted into the codebase, you must complete the |
| 49 [Individual Contributor License | 56 [Individual Contributor License |
| 50 Agreement](http://code.google.com/legal/individual-cla-v1.0.html). You can do | 57 Agreement](http://code.google.com/legal/individual-cla-v1.0.html). You can do |
| 51 this online, and it only takes a minute. If you are contributing on behalf of a | 58 this online, and it only takes a minute. If you are contributing on behalf of a |
| 52 corporation, you must fill out the [Corporate Contributor License Agreement](htt p://code.google.com/legal/corporate-cla-v1.0.html) | 59 corporation, you must fill out the [Corporate Contributor License |
| 53 and send it to us as described on that page. Add your (or your organization's) | 60 Agreement](http://code.google.com/legal/corporate-cla-v1.0.html) |
| 61 and send it to us as described on that page. Add your (or your organization's) | |
| 54 name and contact info to the AUTHORS file as a part of your CL. | 62 name and contact info to the AUTHORS file as a part of your CL. |
| 55 | 63 |
| 56 Now that you've made a change and written a test for it, it's ready for the code | 64 Now that you've made a change and written a test for it, it's ready for the code |
| 57 review! Submit a patch and getting it reviewed is fairly easy with depot tools. | 65 review! Submit a patch and getting it reviewed is fairly easy with depot tools. |
| 58 | 66 |
| 59 Use git-cl, which comes with [depot tools](http://sites.google.com/a/chromium.or g/dev/developers/how-tos/install-depot-tools). | 67 Use git-cl, which comes with [depot |
| 68 tools](http://sites.google.com/a/chromium.org/dev/developers/how-tos/install-dep ot-tools). | |
| 60 For help, run git-cl help. | 69 For help, run git-cl help. |
| 61 | 70 |
| 62 ### Configuring git-cl | 71 ### Configuring git-cl |
| 63 | 72 |
| 64 Before using any git-cl commands you will need to configure it to point at the | 73 Before using any git-cl commands you will need to configure it to point at the |
| 65 correct code review server. This is accomplished with the following command: | 74 correct code review server. This is accomplished with the following command: |
| 66 | 75 |
| 67 ~~~~ | 76 git cl config https://skia.googlesource.com/skia/+/master/codereview.setting s |
| 68 git cl config https://skia.googlesource.com/skia/+/master/codereview.settings | |
| 69 ~~~~ | |
| 70 | 77 |
| 71 ### Find a reviewer | 78 ### Find a reviewer |
| 72 | 79 |
| 73 Ideally, the reviewer is someone who is familiar with the area of code you are | 80 Ideally, the reviewer is someone who is familiar with the area of code you are |
| 74 touching. If you have doubts, look at the git blame for the file to see who else | 81 touching. If you have doubts, look at the git blame for the file to see who else |
| 75 has been editing it. | 82 has been editing it. |
| 76 | 83 |
| 77 ### Uploading changes for review | 84 ### Uploading changes for review |
| 78 | 85 |
| 79 Skia uses Chromium's code review [site](http://codereview.chromium.org) and the | 86 Skia uses Chromium's code review [site](http://codereview.chromium.org) and the |
| 80 Rietveld open source code review tool. | 87 Rietveld open source code review tool. |
| 81 Use git cl to upload your change: | 88 Use git cl to upload your change: |
| 82 ~~~~ | 89 |
| 83 $ git cl upload | 90 $ git cl upload |
| 84 ~~~~ | |
| 85 | 91 |
| 86 You may have to enter a Google Account username and password to authenticate | 92 You may have to enter a Google Account username and password to authenticate |
| 87 yourself to codereview.chromium.org. A free gmail account will do fine, or any | 93 yourself to codereview.chromium.org. A free gmail account will do fine, or any |
| 88 other type of Google account. It does not have to match the email address you | 94 other type of Google account. It does not have to match the email address you |
| 89 configured using git config --global user.email above, but it can. | 95 configured using `git config --global user.email` above, but it can. |
| 90 | 96 |
| 91 The command output should include a URL, similar to | 97 The command output should include a URL, similar to |
| 92 (https://codereview.chromium.org/111893004/), indicating where your changelist | 98 (https://codereview.chromium.org/111893004/), indicating where your changelist |
| 93 can be reviewed. | 99 can be reviewed. |
| 94 | 100 |
| 95 ### Request review | 101 ### Request review |
| 96 | 102 |
| 97 Go to the supplied URL or go to the code review page and click **Issues created | 103 Go to the supplied URL or go to the code review page and click **Issues created |
| 98 by me**. Select the change you want to submit for review and click **Edit | 104 by me**. Select the change you want to submit for review and click **Edit |
| 99 Issue**. Enter at least one reviewer's email address and click **Update Issue**. | 105 Issue**. Enter at least one reviewer's email address and click **Update Issue**. |
| (...skipping 23 matching lines...) Expand all Loading... | |
| 123 | 129 |
| 124 You will likely get email back from the reviewer with comments. Fix these and | 130 You will likely get email back from the reviewer with comments. Fix these and |
| 125 update the patch set in the issue by uploading again. The upload will explain | 131 update the patch set in the issue by uploading again. The upload will explain |
| 126 that it is updating the current CL and ask you for a message explaining the | 132 that it is updating the current CL and ask you for a message explaining the |
| 127 change. Be sure to respond to all comments before you request review of an | 133 change. Be sure to respond to all comments before you request review of an |
| 128 update. | 134 update. |
| 129 | 135 |
| 130 If you need to update code the code on an already uploaded CL, simply edit the | 136 If you need to update code the code on an already uploaded CL, simply edit the |
| 131 code, commit it again locally, and then run git cl upload again e.g. | 137 code, commit it again locally, and then run git cl upload again e.g. |
| 132 | 138 |
| 133 ~~~~ | 139 $ echo "GOATS" > whitespace.txt |
| 134 echo "" > GOATS | 140 $ git add whitespace.txt |
| 135 git add GOATS | 141 $ git commit -m 'add GOATS fix to whitespace.txt' |
| 136 git commit -m 'add newline fix to GOATS' | 142 $ git cl upload |
| 137 git cl upload | |
| 138 ~~~~ | |
| 139 | 143 |
| 140 Once you're ready for another review, use **Publish+Mail Comments** again to | 144 Once you're ready for another review, use **Publish+Mail Comments** again to |
| 141 send another notification (it is helpful to tell the review what you did with | 145 send another notification (it is helpful to tell the review what you did with |
| 142 respect to each of their comments). When the reviewer is happy with your patch, | 146 respect to each of their comments). When the reviewer is happy with your patch, |
| 143 they will say "LGTM" ("Looks Good To Me"). | 147 they will say "LGTM" ("Looks Good To Me"). |
| 144 | 148 |
| 145 _Note_: As you work through the review process, both you and your reviewers | 149 _Note_: As you work through the review process, both you and your reviewers |
| 146 should converse using the code review interface, and send notes using | 150 should converse using the code review interface, and send notes using |
| 147 **Publish+Mail Comments**. | 151 **Publish+Mail Comments**. |
| 148 | 152 |
| 153 Once your change has received an LGTM ("looks good to me"), you can check the | |
|
hcm
2015/11/09 18:23:55
okay to not spell out "looks good to me" again sin
hal.canary
2015/11/09 19:45:50
done
| |
| 154 "Commit" box on the codereview page and it will be committed on your behalf. | |
| 155 | |
| 149 Once your commit has gone in, you should delete the branch containing your chang e: | 156 Once your commit has gone in, you should delete the branch containing your chang e: |
| 150 | 157 |
| 151 ~~~~ | 158 $ git checkout -q origin/master |
| 152 $ git checkout master | 159 $ git branch -D my_feature |
| 153 $ git branch -D my_feature | |
| 154 ~~~~ | |
| 155 | 160 |
| 156 | 161 |
| 157 Final Testing | 162 Final Testing |
| 158 ------------- | 163 ------------- |
| 159 | 164 |
| 160 Skia's principal downstream user is Chromium, and any change to Skia rendering | 165 Skia's principal downstream user is Chromium, and any change to Skia rendering |
| 161 output can break Chromium. If your change alters rendering in any way, you are | 166 output can break Chromium. If your change alters rendering in any way, you are |
| 162 expected to test for and alleviate this. (You may be able to find a Skia team | 167 expected to test for and alleviate this. (You may be able to find a Skia team |
| 163 member to help you, but the onus remains on each individual contributor to avoid | 168 member to help you, but the onus remains on each individual contributor to avoid |
| 164 breaking Chrome. | 169 breaking Chrome. |
| (...skipping 29 matching lines...) Expand all Loading... | |
| 194 please ask a Skia committer to land your patch for you or land using the commit | 199 please ask a Skia committer to land your patch for you or land using the commit |
| 195 queue. | 200 queue. |
| 196 | 201 |
| 197 As part of this process, the Skia committer may create a new codereview | 202 As part of this process, the Skia committer may create a new codereview |
| 198 containing your patch (perhaps with some small adjustments at her discretion). | 203 containing your patch (perhaps with some small adjustments at her discretion). |
| 199 If so, you can mark your codereview as "Closed", and update it with a link to | 204 If so, you can mark your codereview as "Closed", and update it with a link to |
| 200 the new codereview. | 205 the new codereview. |
| 201 | 206 |
| 202 ### Skia committers: | 207 ### Skia committers: |
| 203 * tips on how to apply the externally provided patch are [here](./patch) | 208 * tips on how to apply the externally provided patch are [here](./patch) |
| 204 * when landing externally contributed patches, please note the original | 209 * when landing externally contributed patches, please note the original |
| 205 contributor's identity (and provide a link to the original codereview) in t he commit message | 210 contributor's identity (and provide a link to the original codereview) in t he commit message |
| 206 | 211 |
| 207 git-cl will squash all your commits into a single one with the description you u sed when you uploaded your change. | 212 git-cl will squash all your commits into a single one with the description you u sed when you uploaded your change. |
| 208 | 213 |
| 209 ~~~~ | 214 ~~~~ |
| 210 git cl land | 215 git cl land |
| 211 ~~~~ | 216 ~~~~ |
| 212 or | 217 or |
| 213 ~~~~ | 218 ~~~~ |
| 214 git cl land -c 'Contributor Name <email@example.com>' | 219 git cl land -c 'Contributor Name <email@example.com>' |
| 215 ~~~~ | 220 ~~~~ |
| OLD | NEW |