Bug 50437 - [skia] ignore transform for canvas shadows
Summary: [skia] ignore transform for canvas shadows
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-12-02 21:13 PST by Mike Lawther
Modified: 2010-12-13 21:53 PST (History)
4 users (show)

See Also:

Patch (2.24 KB, patch)
2010-12-02 21:18 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (3.66 KB, patch)
2010-12-09 21:21 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (3.66 KB, patch)
2010-12-13 19:13 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2010-12-13 20:53 PST, Mike Lawther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2010-12-02 21:13:06 PST
[skia] ignore transform for canvas shadows
Comment 1 Mike Lawther 2010-12-02 21:18:19 PST
Created attachment 75462 [details]
Comment 2 Stephen White 2010-12-03 12:24:44 PST
Comment on attachment 75462 [details]

Are there any canvas layout tests which cover this functionality (perhaps in canvas/philip)?  If not, one should be included with this patch.
Comment 3 Mike Lawther 2010-12-05 14:27:15 PST
Sorry - I should have mentioned this. There are existing tests, but not in canvas/philip. The downstream chromium bug I'm addressing is http://code.google.com/p/chromium/issues/detail?id=64647.

The tests are:

Comment 4 Mike Lawther 2010-12-09 21:21:30 PST
Created attachment 76161 [details]
Comment 5 Mike Lawther 2010-12-09 21:28:29 PST
Correcting my previous comment - the test that this patch fixes (on Chromium Win/Linux) are:


fast/canvas/canvas-scale-strokePath-shadow.html now renders the shadow in the correct location, but the pixels don't render right due to another skia bug.
Comment 6 WebKit Review Bot 2010-12-09 21:36:03 PST
Attachment 76161 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6911027
Comment 7 Mike Lawther 2010-12-09 21:43:50 PST
This is only expected to build as of Skia r631. Chromium's DEPS were updated in http://src.chromium.org/viewvc/chrome?view=rev&revision=68558. 

The bot's output says: "Failed to run "['WebKitTools/Scripts/build-webkit', '--release', '--chromium', '--update-chromium']" exit_code: 2". I guess this is the reason the build failed?
Comment 8 Mike Lawther 2010-12-09 21:46:55 PST
Oops - I realise I can't tell whether the --update-chromium failed or not.

Is there something else I need to do to make this patch compile for chromium?
Comment 9 Adam Barth 2010-12-09 21:48:28 PST
Dunno.  Mihaip recently changed how the EWS bot updates its DEPS.
Comment 10 Mihai Parparita 2010-12-09 22:36:53 PST
(In reply to comment #9)
> Dunno.  Mihaip recently changed how the EWS bot updates its DEPS.

I don't think that should matter, when --update-chromium is passed the behavior is the same as it was before my change.

In this case, the Chromium change that Mike mentions is r68558, but the DEPS file that lives in WebKit/chromium/DEPS is only at r67980: http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS. You also need to roll that forward so that upstream Chromium builds succeed.

Upstream Chromium DEPS rolls are not as big of a deal as downstream WebKit rolls (i.e. they're done on-demand, there's no gardening), though it's probably a good idea to do it separately from this patch.
Comment 11 Eric Seidel (no email) 2010-12-13 00:25:43 PST
Comment on attachment 76161 [details]

LGTM.  I assume this is waiting on a Chromium DEPS update?  You should probably post a new patch to make sure the cr-linux ews likes it.  Currently there is no way to make the ewses retry patches.
Comment 12 Mike Lawther 2010-12-13 19:13:50 PST
Created attachment 76486 [details]
Comment 13 Mike Lawther 2010-12-13 20:53:38 PST
Created attachment 76491 [details]
Comment 14 Mike Lawther 2010-12-13 20:57:56 PST
Comment on attachment 76491 [details]

Original patch no longer applies - updated so it does.
Comment 15 Daniel Bates 2010-12-13 21:35:54 PST
Comment on attachment 76491 [details]

This change looks sane to me and seems sane to Eric Seidel as per comment 11. And the Chromium DEPs were updated in changeset 73995 <http://trac.webkit.org/changeset/73995> (bug #50984).
Comment 16 WebKit Review Bot 2010-12-13 21:53:19 PST
Comment on attachment 76491 [details]

Clearing flags on attachment: 76491

Committed r74001: <http://trac.webkit.org/changeset/74001>
Comment 17 WebKit Review Bot 2010-12-13 21:53:25 PST
All reviewed patches have been landed.  Closing bug.