From fd1ad48d4d9073b90f58e60219637046a8446267 Mon Sep 17 00:00:00 2001 From: hcm Date: Wed, 21 Jan 2015 12:05:47 -0800 Subject: Add Contributing to Skia section of docs BUG=skia: Review URL: https://codereview.chromium.org/844433004 --- site/dev/contrib/cqkeywords.md | 96 +++++++ site/dev/contrib/flatten.md | 88 +++++++ site/dev/contrib/index.md | 48 ++++ site/dev/contrib/patch.md | 72 ++++++ site/dev/contrib/revert.md | 18 ++ site/dev/contrib/style.md | 559 +++++++++++++++++++++++++++++++++++++++++ site/dev/contrib/submit.md | 248 ++++++++++++++++++ 7 files changed, 1129 insertions(+) create mode 100644 site/dev/contrib/cqkeywords.md create mode 100644 site/dev/contrib/flatten.md create mode 100644 site/dev/contrib/index.md create mode 100644 site/dev/contrib/patch.md create mode 100644 site/dev/contrib/revert.md create mode 100644 site/dev/contrib/style.md create mode 100644 site/dev/contrib/submit.md diff --git a/site/dev/contrib/cqkeywords.md b/site/dev/contrib/cqkeywords.md new file mode 100644 index 0000000000..6c834e235e --- /dev/null +++ b/site/dev/contrib/cqkeywords.md @@ -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. diff --git a/site/dev/contrib/flatten.md b/site/dev/contrib/flatten.md new file mode 100644 index 0000000000..c06a14b812 --- /dev/null +++ b/site/dev/contrib/flatten.md @@ -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): + + +~~~~ +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: + + +~~~~ +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: + + +~~~~ +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: + + +~~~~ +public: + +SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP() +~~~~ + +Then in the cpp file you define all the members of the group together: + + +~~~~ +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 + + +~~~~ +SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkNewClass) +~~~~ + +For a group, add + + +~~~~ +SkGroupClass::InitializeFlattenables(); +~~~~ + diff --git a/site/dev/contrib/index.md b/site/dev/contrib/index.md new file mode 100644 index 0000000000..f1c730584d --- /dev/null +++ b/site/dev/contrib/index.md @@ -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. + diff --git a/site/dev/contrib/patch.md b/site/dev/contrib/patch.md new file mode 100644 index 0000000000..cf8b35bdeb --- /dev/null +++ b/site/dev/contrib/patch.md @@ -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 + * git log + * + * git revert + * git cl upload + * git cl land diff --git a/site/dev/contrib/style.md b/site/dev/contrib/style.md new file mode 100644 index 0000000000..dd39a734a8 --- /dev/null +++ b/site/dev/contrib/style.md @@ -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. + + +~~~~ +class SkClass { +public: + class HelperClass { + ... + }; +}; +~~~~ + +Data fields in structs, classes, unions begin with lowercase f and are then +camel capped. + + +~~~~ +struct GrCar { + ... + float fMilesDriven; + ... +}; +~~~~ + +Globals variables are similar but prefixed with g and camel-capped + + +~~~~ +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 +kCount and not be a member of the enum (see example): + + +~~~~ +enum SkPancakeType { + kBlueberry_PancakeType, + kPlain_PancakeType, + kChocolateChip_PancakeType, + + kLast_PancakeType = kChocolateChip_PancakeType +}; + +static const SkPancakeType kPancakeTypeCount = kLast_PancakeType + 1; +~~~~ + +A bitfield: + + +~~~~ +enum SkSausageIngredientBits { + kFennel_SuasageIngredientBit = 0x1, + kBeef_SausageIngredientBit = 0x2 +}; +~~~~ + +or: + + +~~~~ +enum SkMatrixFlags { + kTranslate_MatrixFlag = 0x1, + kRotate_MatrixFlag = 0x2 +}; +~~~~ + +Exception: anonymous enums can be used to declare integral constants, e.g.: + + +~~~~ +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: + + +~~~~ +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: + + +~~~~ +bool SkIsOdd(int n); + +class SkFoo { +public: + static int FooInstanceCount(); +}; +~~~~ + +Macros +------ + +Ganesh macros that are GL-specific should be prefixed GR_GL. + + +~~~~ +#define GR_GL_TEXTURE0 0xdeadbeef +~~~~ + +Ganesh prefers that macros are always defined and the use of #if MACRO rather than +#ifdef MACRO. + + +~~~~ +#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. + + +~~~~ +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: + + +~~~~ +while (...) { +} + +do { +} while(...); + +switch (...) { +... +} +~~~~ + +Cases and default in switch statements are indented from the switch. + + +~~~~ +switch (color) { + case kBlue: + ... + break; + case kGreen: + ... + break; + ... + default: + ... + break; +} +~~~~ + +Fallthrough from one case to the next is commented unless it is trivial: + + +~~~~ +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: + + +~~~~ +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. + + +~~~~ +class SkFoo { + +public: + ... + +protected: + ... + +private: + SkBar fBar; + ... + + void barHelper(...); + ... +}; +~~~~ + +Subclasses should have a private typedef of their super class called INHERITED: + + +~~~~ +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. + + +~~~~ +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: + + +~~~~ +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! + + +~~~~ +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. + + +~~~~ +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: + + +~~~~ +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. + + +~~~~ +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: + + +~~~~ +if (7 == luckyNumber) { + ... +} +~~~~ + +However, inequality operators need not follow this rule: + + +~~~~ +if (count > 0) { + ... +} +~~~~ + +Comments + +We use doxygen-style comments. + +For grouping or separators in an implementation file we use 80 slashes + + +~~~~ +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): + + +~~~~ +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: + + +~~~~ +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 + + +~~~~ +void modify_foo(SkFoo* foo) { + // Modify foo +} +~~~~ + +Then the function can be called as followed: + + +~~~~ +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). + + +~~~~ +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. + + +~~~~ +// 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 + + +~~~~ +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 + + +~~~~ +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). + diff --git a/site/dev/contrib/submit.md b/site/dev/contrib/submit.md new file mode 100644 index 0000000000..fcbcf2077d --- /dev/null +++ b/site/dev/contrib/submit.md @@ -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: + + +~~~~ +/* + * 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. + + +~~~~ +'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 ' +~~~~ -- cgit v1.2.3