From 193b1346552a49460488274cb91799c8b0c78181 Mon Sep 17 00:00:00 2001 From: hcm Date: Wed, 18 Feb 2015 12:56:40 -0800 Subject: [PATCH] managing chromes use section of docs BUG=skia: Review URL: https://codereview.chromium.org/934893003 --- site/dev/chrome/blink.md | 101 +++++++++++++++++++++++++++++++++++++++++++++ site/dev/chrome/changes.md | 35 ++++++++++++++++ site/dev/chrome/index.md | 5 +++ site/dev/chrome/repo.md | 19 +++++++++ 4 files changed, 160 insertions(+) create mode 100644 site/dev/chrome/blink.md create mode 100644 site/dev/chrome/changes.md create mode 100644 site/dev/chrome/index.md create mode 100644 site/dev/chrome/repo.md diff --git a/site/dev/chrome/blink.md b/site/dev/chrome/blink.md new file mode 100644 index 0000000..4889440 --- /dev/null +++ b/site/dev/chrome/blink.md @@ -0,0 +1,101 @@ +How to land Skia changes that change Blink layout test results +============================================================== + +Changes that affect a small number of layout test results +--------------------------------------------------------- +Changes affecting fewer than ~20 layout tests can be rebaselined without special +coordination with the Blink gardener using these steps: + +1. Prepare your Skia change, taking note of which layout tests will turn red + \(see http://www.chromium.org/developers/testing/webkit-layout-tests for more + detail on running the Blink layout tests\). +2. Check in your code to the Skia repo. +3. Ahead of the Skia auto roll including your change, manually push a change to the + Blink LayoutTests/TestExpectations [file](http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations), + flagging tests expected to fail as a result of your change with \[ NeedsManualRebaseline \]. +4. Wait for the Skia roll to land successfully. +5. Check in another change to the Blink TestExpectations file changing the flags to + \[ NeedsRebaseline\], which will prompt the automatic rebaseline. + + + +Changes that affect a large number of test results +-------------------------------------------------- +Where a 'large number' or 'many' means more than about 20. +Follow the instructions below: + +In the following the term 'code suppression' means a build flag \(a\.k\.a\. define\). +Such code suppressions should be given a name with the form SK\_IGNORE\_xxx\_FIX. + +There are dependency revisions which must be updated in this +process. Updating a dependency revision is called a 'roll'. +There are two different rolls which concern this process, each of which happens +via an Auto Roll Bot multiple times per day, and can also be done manually: + + * Skia roll into Chromium. See +https://chromium.googlesource.com/chromium/src/+log/master/DEPS and search for +skia\-deps\-roller. + * Blink roll into Chromium. See +https://chromium.googlesource.com/chromium/src/+log/master/DEPS and search for +blink\-deps\-roller. + +### Setup +#### Code suppression does not yet exist \- Direct method +1. Make a change in Skia which will change many Blink layout tests. +2. Put the change behind a code suppression. +3. Check in the change to the Skia repository. +4. Manually roll Skia or append the autoroll with the code suppression to + Chromium's 'skia/skia\_common\.gypi' +5. Add code suppression to Blink's 'public/blink\_skia\_config\.gyp'. +6. Wait for Blink roll into Chromium. +7. Remove code suppression from Chromium's 'skia/skia\_common\.gypi'. + +#### Code suppression does not yet exist \- Alternate method +1. Add code suppression to Blink's 'public/blink\_skia\_config\.gyp' before making code + changes in Skia. +2. Make a change in Skia which will change many Blink layout tests. +3. Put the change behind a code suppression. +4. Wait for Blink roll into Chromium. +5. Check in the change to the Skia repository. +6. Wait for Skia roll into Chromium. + +#### Code suppression exists in header +1. Remove code suppression from header file in Chromium and add code suppression to + Chromium's 'skia/skia\_common\.gypi'. + The code suppression cannot be in a header file and a defined in a gyp file at the + same time or a multiple definition warning will be treated as an error and break + the Chromium build. +2. Add code suppression to Blink's 'public/blink\_skia\_config\.gyp'. +3. Wait for Blink roll into Chromium. +4. Remove code suppression from Chromium's 'skia/skia\_common\.gypi'. + +### Rebaseline +1. Choose a time when the Blink tree is likely to be quiet. Avoid PST afternoons in + particular. The bigger the change, the more important this is. Regardless, + determine who the Blink gardener is and notify them. You will be making the + Chromium\.WebKit tree very red for an extended period, and the gardener needs to + know that they are not expected to fix it. +2. Create a CL removing the code suppression from Blink's + public/blink_skia_config.gyp while simultaneously adding [ NeedsRebaseline ] + lines to Blink's LayoutTests/TestExpectations [file](http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations). + Then the auto rebaseline bot will take care of the work of actually checking in the + new images. This is generally acceptable for up to 600 or so rebaselined images. + Above that you might still use [ NeedsRebaseline ], but it's best to coordinate with + the gardener/sheriff. This should go through the CQ cleanly. +3. Be careful with tests that are already failing or flakey. These may or may not need + to be rebaselined and flakey tests should not be removed from TestExpectations + regardless. In such cases revert the TestExpectations changes before committing. +4. If you are not the one handling the cleanup step, please open a Skia Issue of the + form + Title: "Remove code suppression SK\_IGNORE\_xxx\_FIX\." + Comment: "Code suppression SK\_IGNORE\_xxx\_FIX rebaselined with Blink revision + 123456\." and assign it to the individual responsible for the cleanup step. + +### Cleanup +1. Wait for Blink roll into Chromium, so that Chromium is using the new Skia code + and new Blink baselines. +2. Remove the now unused old code from Skia and any defines which were introduced + to suppress the new code. +3. Check in the cleanup change to the Skia repository. +4. Wait for Skia roll into Chromium. + diff --git a/site/dev/chrome/changes.md b/site/dev/chrome/changes.md new file mode 100644 index 0000000..5ded657 --- /dev/null +++ b/site/dev/chrome/changes.md @@ -0,0 +1,35 @@ +Landing Skia changes which require associated Chrome changes +============================================================ + +If your change modifies the Skia API, you may also need to land a change in Chromium. + +The strategy you use to synchronize changes in the Skia and Chromium +repositories may differ based on the nature of the change, but in general, we +recommend using build flag suppressions \(defines\)\. +We also prefer making the old code path opt-in where possible. + +Method 1 \(preferred\) \- Make the old code path opt\-in for Chromium + + * Add new code to Skia, leaving the old code in place. + * Deprecate the old code path so that it must be enabled with a flag such as + 'SK_SUPPORT_LEGACY_XXX'. + * Synchronize the above changes in Skia with a Chromium commit to + 'skia/skia_common.gypi' or 'skia/config/SkUserConfig.h' to enable the + deprecated Skia API. + * Note that the code suppression cannot exist in both the header file and + the gyp file, it should only reside in one location. + * Test the new or updated Skia API within Chromium. + * Remove the flag and code when the legacy code path is no longer in use. + +Method 2 \- Make the new code path opt\-in for Chromium + + * Add new code to Skia, suppressed by a flag. + * Leave the old code path in place. + * Set the flag in Chromium's 'skia/skia_common.gypi' or + 'skia/config/SkUserConfig.h' to enable the new or updated Skia API. + * Test the new or updated Skia API within Chromium. + * Remove the code suppression \(and code\) when the legacy API is no longer + in use. + +If your changes will affect Blink layout tests, see detailed instructions about +how to synchronize the changes between Skia, Blink, and Chromium [here](./blink). diff --git a/site/dev/chrome/index.md b/site/dev/chrome/index.md new file mode 100644 index 0000000..a3c7ca4 --- /dev/null +++ b/site/dev/chrome/index.md @@ -0,0 +1,5 @@ +Managing Chrome's use of Skia +============================= + +If you have a Skia change that needs to be tested in Chrome or Blink, or which +requires associated changes in those repositories, this is the place for you. diff --git a/site/dev/chrome/repo.md b/site/dev/chrome/repo.md new file mode 100644 index 0000000..16cea6c --- /dev/null +++ b/site/dev/chrome/repo.md @@ -0,0 +1,19 @@ +How to manage a Skia repo inside a Chromium repo +================================================ + +To work on Skia inside a Chromium checkout, run the following: + +~~~~ +$ cd chromium/src/third_party/skia +$ tools/git-sync-deps +~~~~ + +This command does a minimal "just sync the DEPS" emulation of gclient sync for +Skia into chromium/src/third_party/skia/third_party. After that, make dm or +./gyp_skia && ninja -C out/Debug dm in chromium/src/third_party/skia will get +you rolling. + +We no longer recommend the .gclient file manipulation to have Chromium DEPS also +sync Skia's DEPS. Most of those DEPS are for building and testing only; +Chromium doesn't need any of them, and it can be confusing and problematic if +they somehow get mixed into the Chromium build. -- 2.7.4