AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Add Contributing to Skia section of docs

Browse Source 
BUG=skia:

Review URL: https://codereview.chromium.org/844433004
This commit is contained in:
hcm 2015-01-21 12:05:47 -08:00 committed by Commit bot
parent ec7e12f3cf
commit fd1ad48d4d
7 changed files with 1129 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
site/dev/contrib/cqkeywords.md Normal file
View File

@ -0,0 +1,96 @@
Commit Queue Keywords
=====================
COMMIT
------
If you want to test your CL through the commit queue but are not ready to commit
the changes yet, you can add the following line to the CL description:
COMMIT=false
The CQ will run through its list of verifiers (reviewer check, trybots, tree check,
presubmit check), and will close the issue instead of committing it.
CQ_INCLUDE_TRYBOTS
Allows you to add arbitrary trybots to the CQ's list of default trybots.
The CQ will block till these tryjobs pass just like the default list of tryjobs.
This is the format of the values of this keyword:
CQ_INCLUDE_TRYBOTS=master1:bot1,bot2;master2:bot3,bot4
Here are some real world examples:
CQ_INCLUDE_TRYBOTS=tryserver.chromium:linux_layout_rel
CQ_INCLUDE_TRYBOTS=tryserver.skia:Build-Ubuntu13.10-GCC4.8-NaCl-Release-Trybot
CQ_EXCLUDE_TRYBOTS
Allows you to remove trybots from the CQ's list of default trybots. Should only be
used when particular builders are failing for reasons unrelated to your code changes.
This is the format of the values of this keyword:
CQ_EXCLUDE_TRYBOTS=master1:bot1,bot2;master2:bot3,bot4
Here are some real world examples:
CQ_EXCLUDE_TRYBOTS=tryserver.chromium:win_chromium_compile_dbg
CQ_EXCLUDE_TRYBOTS=tryserver.skia:Build-Win7-VS2010-x86-Debug-Trybot
CQ_TRYBOTS
Allows you to list every trybot that you want to run for your CL.
This is the format of the values of this keyword:
CQ_TRYBOTS=master1:bot1,bot2;master2:bot3,bot4
Here are some real world examples:
CQ_TRYBOTS=tryserver.chromium:linux_chromium_gn_rel,linux_chromium_chromeos_rel,
android_dbg_triggered_tests,android_dbg,mac_chromium_rel,win_chromium_x64_rel
CQ_TRYBOTS=tryserver.skia:Build-Win7-VS2010-x86-Debug-Trybot,
Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot,
Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot,
Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot,Build-Mac10.8-Clang-x86_64-Release-Trybot
TBR
---
If you are a Skia committer and cannot wait for a review,
then you can include the TBR keyword in your CL's description.
Example:
TBR=rmistry@google.com
NOTREECHECKS
If you want to skip the tree status checks, to make the CQ commit a CL even if the tree is closed,
you can add the following line to the CL description:
NOTREECHECKS=true
This is discouraged, since the tree is closed for a reason. However, in rare cases this is acceptable,
primarily to fix build breakages (i.e., your CL will help in reopening the tree).
NOPRESUBMIT
If you want to skip the presubmit checks, add the following line to the CL description:
NOPRESUBMIT=true
NOTRY
-----
If you cannot wait for the try job results, you can add the following line to the CL description:
NOTRY=true
The CQ will then not run any try jobs for your change and will commit the CL as soon as the tree is open, assuming the presubmit check passes.

88
site/dev/contrib/flatten.md Normal file
View File

@ -0,0 +1,88 @@
Flattenables
============
Many objects in Skia, such as SkShaders and other effects on SkPaint, need to be
flattened into a data stream for either transport or as part of the key to the
font cache. Classes for these objects should derive from SkFlattenable or one of
its subclasses. If you create a new flattenable class, you need to make sure you
do a few things so that it will work on all platforms:
1: Override the method flatten (the default scope is protected):
<!--?prettify?-->
~~~~
virtual void flatten(SkFlattenableWriteBuffer& buffer) const SK_OVERRIDE {
this->INHERITED::flatten(buffer);
// Write any private data that needs to be stored to recreate this object
}
~~~~
2: Override the (protected) constructor that creates an object from an
SkFlattenableReadBuffer:
<!--?prettify?-->
~~~~
SkNewClass(SkFlattenableReadBuffer& buffer)
: INHERITED(buffer) {
// Read the data from the buffer in the same order as it was written to the
// SkFlattenableWriteBuffer and construct the new object
}
~~~~
3: Declare a set of deserialization procs for your object in the class declaration:
We have a macro for this:
<!--?prettify?-->
~~~~
public:
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkNewClass)
~~~~
4: If your class is declared in a .cpp file or in a private header file, create a
function to register its group:
This occurs in cases where the classes are hidden behind a factory, like many effects
and shaders are. Then in the parent class header file (such as SkGradientShader) you
need to add:
<!--?prettify?-->
~~~~
public:
SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP()
~~~~
Then in the cpp file you define all the members of the group together:
<!--?prettify?-->
~~~~
SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkGroupClass)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkMemberClass1)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkMemberClass2)
// etc
SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_END
~~~~
5: Register your flattenable with the global registrar:
You need to add one line to SkFlattenable::InitalizeFlattenables(). To register the
flattenable in a Skia build, that function is defined in SkGlobalInitialization_default.cpp.
For Chromium, it is in SkGlobalInitialization_chromium.cpp.
For a single flattenable add
<!--?prettify?-->
~~~~
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkNewClass)
~~~~
For a group, add
<!--?prettify?-->
~~~~
SkGroupClass::InitializeFlattenables();
~~~~

48
site/dev/contrib/index.md Normal file
View File

@ -0,0 +1,48 @@
Contributing to Skia
====================
Here some ways you can get involved and help us improve Skia.
Report Bugs
-----------
Find bugs to fix or report new bugs in the [Skia issue tracker](http://code.google.com/p/skia/issues/list).
You can also search the [Chromium issue tracker](http://code.google.com/p/chromium/issues/list) for bugs related to graphics or Skia.
Test
----
Write an application or tool that will exercise the Skia code differently than our
current set of tests and verify that Skia works as expected. Draw something
interesting and profile it to find ways to speed up Skia's implementation.
We cannot always fix issues or support every scenario, but we welcome any bugs
found so we can assess and prioritize them. (If you find _and_ fix a bug, even better!)
Contribute Code
---------------
Whether you develop a new feature or a fix for an existing bug in the Skia code base,
you will need a committer to review and approve the change. There are some steps that
can speed up the review process:
Keep your code submissions small and targeted.
When possible, have a fellow contributor review your change in advance of submission.
Propose new features to the project leads by opening a feature bug or posting to
skia-discuss ahead of development. For more information, see [How to submit a patch](./submit).
For background on the project and an outline of the types of roles interested parties
can take on, see [Project Roles](../../roles).
Anyone contributing code to Skia must sign a Contributor License Agreement and ensure
they are listed in the AUTHORS file:
Individual contributors can complete the [Individual Contributor License Agreement](https://developers.google.com/open-source/cla/individual) online.
If you are contributing on behalf of a corporation, fill out the [Corporate Contributor License Agreement](https://developers.google.com/open-source/cla/corporate)
and send it in as described on that page.
If it is your first time submitting code or you have not previously done so, add your
(or your organization's) name and contact info to the [AUTHORS file](https://skia.googlesource.com/skia/+/master/AUTHORS) as a part
of your CL.
REVIEWERS: Before you LGTM a change, verify that the contributor is listed in the AUTHORS file.
If they are not, a Googler must ensure that the individual or their corporation has signed the
CLA by searching [go/cla-signers](https://goto.google.com/cla-signers).
Then have an entry added to the AUTHORS file with the CL.

72
site/dev/contrib/patch.md Normal file
View File

@ -0,0 +1,72 @@
Applying patches
================
If you are a Skia committer and have been asked to commit an
externally-submitted patch, this is how to do it. (This technique is useful in
other situations too, like if you just want to try out somebody else's patch
locally.)
Notes:
* For the examples below, we will assume that this is the change you want
to patch into your local checkout: https://codereview.appspot.com/6201055/
* These instructions should work on Mac or Linux; Windows is trickier,
because there is no standard Windows "patch" tool.
See also:
http://dev.chromium.org/developers/contributing-code#TOC-Instructions-for-Reviewer:-Checking-in-the-patch-for-a-non-committer
If you use git cl, then you should be able to use the shortcut:
~~~~
git cl patch 6201055
~~~~
If you use gcl, or the above doesn't work, the following should always work.
1. Prepare your local workspace to accept the patch.
* cd into the root directory (usually trunk/) of the workspace where you
want to apply the patch.
* Make sure that the workspace is up-to-date and clean (or "updated and
clean enough" for your purposes). If the codereview patch was against
an old revision of the repo, you may need to sync your local workspace
to that same revision...
2. Download the raw patch set.
* Open the codereview web page and look for the "Download raw patch set"
link near the upper right-hand corner. Right-click on that link and copy
it to the clipboard. (In my case, the link is
https://codereview.appspot.com/download/issue6201055_1.diff )
* If you are on Linux or Mac and have "curl" or "wget" installed, you can
download the patch from the command line:
~~~~
curl https://codereview.appspot.com/download/issue6201055_1.diff
--output patch.txt
# or...
wget https://codereview.appspot.com/download/issue6201055_1.diff
--output-document=patch.txt
~~~~
* Otherwise, figure out some other way to download this file and save it as
'patch.txt'
3. Apply this patch to your local checkout.
* You should still be in the root directory of the workspace where you want
to apply the patch.
~~~~
patch -p1 <patch.txt
~~~~
* Then you can run diff and visually check the local changes.
4. Complications: If the patch fails to apply, the following may be happening:
Wrong revision. Maybe your local workspace is not up to date? Or maybe the
patch was made against an old revision of the repository, and cannot be applied
to the latest revision? (In that case, revert any changes and sync your
workspace to an older revision, then re-apply the patch.)

18
site/dev/contrib/revert.md Normal file
View File

@ -0,0 +1,18 @@
How to revert a CL
==================
Using one-click revert
----------------------
* Find the codereview issue for the CL you want to revert.
* Click the "revert" button.
Using Git
---------
* git checkout master
* git svn fetch && git svn rebase && gclient sync
* git checkout -t -b <branch_name>
* git log
* <Find the SHA1 of the commit you want to revert>
* git revert <SHA1>
* git cl upload
* git cl land

559
site/dev/contrib/style.md Normal file
View File

@ -0,0 +1,559 @@
Coding Style Guidelines
=======================
These conventions have evolved over time. Some of the earlier code in both
projects doesn’t strictly adhere to the guidelines. However, as the code evolves
we hope to make the existing code conform to the guildelines.
Files
-----
We use .cpp and .h as extensions for c++ source and header files. We use
foo_impl.h for headers with inline definitions for class foo.
Headers that aren’t meant for public consumption should be placed in src
directories so that they aren’t in a client’s search path.
We prefer to minimize includes. If forward declaring a name in a header is
sufficient then that is preferred to an include.
Forward declarations and file includes should be in alphabetical order (but we
aren't very strict about it).
Do not use #if/#ifdef before including "SkTypes.h" (directly or indirectly).
We use spaces not tabs (4 of them).
We use Unix style endlines (LF).
We prefer no trailing whitespace but aren't very strict about it.
We wrap lines at 100 columns unless it is excessively ugly (use your judgement).
The soft line length limit was changed from 80 to 100 columns in June 2012. Thus,
most files still adhere to the 80 column limit. It is not necessary or worth
significant effort to promote 80 column wrapped files to 100 columns. Please
don't willy-nilly insert longer lines in 80 column wrapped files. Either be
consistent with the surrounding code or, if you really feel the need, promote
the surrounding code to 100 column wrapping.
Naming
------
Both projects use a prefix to designate that they are Skia prefix for classes,
enums, structs, typedefs etc is Sk. Ganesh’s is Gr. Nested types should not be
prefixed.
<!--?prettify?-->
~~~~
class SkClass {
public:
class HelperClass {
...
};
};
~~~~
Data fields in structs, classes, unions begin with lowercase f and are then
camel capped.
<!--?prettify?-->
~~~~
struct GrCar {
...
float fMilesDriven;
...
};
~~~~
Globals variables are similar but prefixed with g and camel-capped
<!--?prettify?-->
~~~~
bool gLoggingEnabled
Local variables begin lowercases and are camel-capped.
int herdCats(const Array& cats) {
int numCats = cats.count();
}
~~~~
Enum values are prefixed with k and have post fix that consists of an underscore
and singular name of the enum name. The enum itself should be singular for
exclusive values or plural for a bitfield. If a count is needed it is
k<singular enum name>Count and not be a member of the enum (see example):
<!--?prettify?-->
~~~~
enum SkPancakeType {
kBlueberry_PancakeType,
kPlain_PancakeType,
kChocolateChip_PancakeType,
kLast_PancakeType = kChocolateChip_PancakeType
};
static const SkPancakeType kPancakeTypeCount = kLast_PancakeType + 1;
~~~~
A bitfield:
<!--?prettify?-->
~~~~
enum SkSausageIngredientBits {
kFennel_SuasageIngredientBit = 0x1,
kBeef_SausageIngredientBit = 0x2
};
~~~~
or:
<!--?prettify?-->
~~~~
enum SkMatrixFlags {
kTranslate_MatrixFlag = 0x1,
kRotate_MatrixFlag = 0x2
};
~~~~
Exception: anonymous enums can be used to declare integral constants, e.g.:
<!--?prettify?-->
~~~~
enum { kFavoriteNumber = 7 };
~~~~
Macros are all caps with underscores between words. Macros that have greater
than file scope should be prefixed SK or GR.
Static non-class functions in implementation files are lower case with
underscores separating words:
<!--?prettify?-->
~~~~
static inline bool tastes_like_chicken(Food food) {
return kIceCream_Food != food;
}
~~~~
Externed functions or static class functions are camel-capped with an initial cap:
<!--?prettify?-->
~~~~
bool SkIsOdd(int n);
class SkFoo {
public:
static int FooInstanceCount();
};
~~~~
Macros
------
Ganesh macros that are GL-specific should be prefixed GR_GL.
<!--?prettify?-->
~~~~
#define GR_GL_TEXTURE0 0xdeadbeef
~~~~
Ganesh prefers that macros are always defined and the use of #if MACRO rather than
#ifdef MACRO.
<!--?prettify?-->
~~~~
#define GR_GO_SLOWER 0
...
#if GR_GO_SLOWER
Sleep(1000);
#endif
~~~~
Skia tends to use #ifdef SK_MACRO for boolean flags.
Braces
------
Open braces don’t get a newline. “else” and “else if” appear on same line as
opening and closing braces unless preprocessor conditional compilation
interferes. Braces are always used with if, else, while, for, and do.
<!--?prettify?-->
~~~~
if (...) {
oneOrManyLines;
}
if (...) {
oneOrManyLines;
} else if (...) {
oneOrManyLines;
} else {
oneOrManyLines;
}
for (...) {
oneOrManyLines;
}
while (...) {
oneOrManyLines;
}
void function(...) {
oneOrManyLines;
}
if (!error) {
proceed_as_usual();
}
#if HANDLE_ERROR
else {
freak_out();
}
#endif
~~~~
Flow Control
------------
There is a space between flow control words and parentheses and between
parentheses and braces:
<!--?prettify?-->
~~~~
while (...) {
}
do {
} while(...);
switch (...) {
...
}
~~~~
Cases and default in switch statements are indented from the switch.
<!--?prettify?-->
~~~~
switch (color) {
case kBlue:
...
break;
case kGreen:
...
break;
...
default:
...
break;
}
~~~~
Fallthrough from one case to the next is commented unless it is trivial:
<!--?prettify?-->
~~~~
switch (recipe) {
...
case kCheeseOmelette_Recipe:
ingredients |= kCheese_Ingredient;
// fallthrough
case kPlainOmelette_Recipe:
ingredients |= (kEgg_Ingredient | kMilk_Ingredient);
break;
...
}
~~~~
When a block is needed to declare variables within a case follow this pattern:
<!--?prettify?-->
~~~~
switch (filter) {
...
case kGaussian_Filter: {
Bitmap srcCopy = src->makeCopy();
...
break;
}
...
};
~~~~
Classes
-------
Unless there is a need for forward declaring something, class declarations
should be ordered public, protected, private. Each should be preceded by a
newline. Within each visibility section (public, private), fields should not be
intermixed with methods.
<!--?prettify?-->
~~~~
class SkFoo {
public:
...
protected:
...
private:
SkBar fBar;
...
void barHelper(...);
...
};
~~~~
Subclasses should have a private typedef of their super class called INHERITED:
<!--?prettify?-->
~~~~
class GrDillPickle : public GrPickle {
...
private:
typedef GrPickle INHERITED;
};
~~~~
Virtual functions that are overridden in derived classes should use SK_OVERRIDE
(and not the override keyword). The virtual keyword can be omitted.
<!--?prettify?-->
~~~~
void myVirtual() SK_OVERRIDE {
}
~~~~
This should be the last element of their private section, and all references to
base-class implementations of a virtual function should be explicitly qualified:
<!--?prettify?-->
~~~~
void myVirtual() SK_OVERRIDE {
...
this->INHERITED::myVirtual();
...
}
~~~~
As in the above example, derived classes that redefine virtual functions should
use SK_OVERRIDE to note that explicitly.
Constructor initializers should be one per line, indented, with punctuation
placed before the initializer. This is a fairly new rule so much of the existing
code is non-conforming. Please fix as you go!
<!--?prettify?-->
~~~~
GrDillPickle::GrDillPickle()
: GrPickle()
, fSize(kDefaultPickleSize) {
...
}
~~~~
Constructors that take one argument should almost always be explicit, with
exceptions made only for the (rare) automatic compatibility class.
<!--?prettify?-->
~~~~
class Foo {
explicit Foo(int x); // Good.
Foo(float y); // Spooky implicit conversion from float to Foo. No no no!
...
};
~~~~
Method calls within method calls should be prefixed with dereference of the
'this' pointer. For example:
<!--?prettify?-->
~~~~
this->method();
Memory Managemt
~~~~
All memory allocation should be routed through SkNEW and its variants. These are
#defined in SkPostConfig.h, but the correct way to get access to the config
system is to #include SkTypes.h, which will allow external users of the library
to provide a custom memory manager or other adaptations.
<!--?prettify?-->
~~~~
SkNEW(type_name)
SkNEW_ARGS(type_name, args)
SkNEW_ARRAY(type_name, count)
SkNEW_PLACEMENT(buf, type_name)
SkNEW_PLACEMENT_ARGS(buf, type_name, args)
SkDELETE(obj)
SkDELETE_ARRAY(array)
~~~~
Comparisons
-----------
We prefer that equality operators between lvalues and rvalues place the lvalue
on the right:
<!--?prettify?-->
~~~~
if (7 == luckyNumber) {
...
}
~~~~
However, inequality operators need not follow this rule:
<!--?prettify?-->
~~~~
if (count > 0) {
...
}
~~~~
Comments
We use doxygen-style comments.
For grouping or separators in an implementation file we use 80 slashes
<!--?prettify?-->
~~~~
void SkClassA::foo() {
...
}
////////////////////////////////////////////////////////////////
void SkClassB::bar() {
...
}
~~~~
Integer Types
-------------
We follow the Google C++ guide for ints and are slowly making older code conform to this
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types)
Summary: Use int unless you have need a guarantee on the bit count, then use
stdint.h types (int32_t, etc). Assert that counts, etc are not negative instead
of using unsigned. Bitfields use uint32_t unless they have to be made shorter
for packing or performance reasons.
NULL, 0
-------
Use NULL for pointers, 0 for ints. We prefer explicit NULL comparisons when
checking for NULL pointers (as documentation):
<!--?prettify?-->
~~~~
if (NULL == x) { // slightly preferred over if (x)
...
}
~~~~
When checking non-NULL pointers explicit comparisons are not required because it
reads like a double negative:
<!--?prettify?-->
~~~~
if (x) { // slightly preferred over if (NULL != x)
...
}
~~~~
Returning structs
-----------------
If the desired behavior is for a function to return a struct, we prefer using a
struct as an output parameter
<!--?prettify?-->
~~~~
void modify_foo(SkFoo* foo) {
// Modify foo
}
~~~~
Then the function can be called as followed:
<!--?prettify?-->
~~~~
SkFoo foo;
modify_foo(&foo);
~~~~
This way, if return value optimization cannot be used there is no performance
hit. It also means that modify_foo can actually return a boolean for whether the
call was successful. In this case, initialization of foo can potentially be
skipped on failure (assuming the constructor for SkFoo does no initialization).
<!--?prettify?-->
~~~~
bool modify_foo(SkFoo* foo) {
if (some_condition) {
// Modify foo
return true;
}
// Leave foo unmodified
return false;
}
~~~~
Function Parameters
-------------------
Mandatory constant object parameters are passed to functions as const references
if they are not retained by the receiving function. Optional constant object
parameters are passed to functions as const pointers. Objects that the called
function will retain, either directly or indirectly, are passed as pointers.
Variable (i.e. mutable) object parameters are passed to functions as pointers.
<!--?prettify?-->
~~~~
// src and paint are optional
void SkCanvas::drawBitmapRect(const SkBitmap& bitmap, const SkIRect* src,
const SkRect& dst, const SkPaint* paint = NULL);
// metrics is mutable (it is changed by the method)
SkScalar SkPaint::getFontMetrics(FontMetric* metrics, SkScalar scale) const;
// A reference to foo is retained by SkContainer
void SkContainer::insert(const SkFoo* foo);
~~~~
If function arguments or parameters do not all fit on one line, they may be
lined up with the first parameter on the same line
<!--?prettify?-->
~~~~
void drawBitmapRect(const SkBitmap& bitmap, const SkRect& dst,
const SkPaint* paint = NULL) {
this->drawBitmapRectToRect(bitmap, NULL, dst, paint,
kNone_DrawBitmapRectFlag);
}
~~~~
or placed on the next line indented eight spaces
<!--?prettify?-->
~~~~
void drawBitmapRect(
const SkBitmap& bitmap, const SkRect& dst,
const SkPaint* paint = NULL) {
this->drawBitmapRectToRect(
bitmap, NULL, dst, paint, kNone_DrawBitmapRectFlag);
}
~~~~
Python
------
Python code follows the [Google Python Style Guide](http://google-styleguide.googlecode.com/svn/trunk/pyguide.html).

248
site/dev/contrib/submit.md Normal file
View File

@ -0,0 +1,248 @@
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:
<!--?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.
<!--?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>'
~~~~