| Summary: | [skia] ignore transform for canvas shadows | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mike Lawther <mikelawther> | ||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, mihaip, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Other | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Mike Lawther
2010-12-02 21:13:06 PST
Created attachment 75462 [details]
Patch
Comment on attachment 75462 [details]
Patch
Are there any canvas layout tests which cover this functionality (perhaps in canvas/philip)? If not, one should be included with this patch.
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: fast/canvas/canvas-scale-fillRect-shadow.html fast/canvas/canvas-scale-fillPath-shadow.html fast/canvas/canvas-scale-strokePath-shadow.html Created attachment 76161 [details]
Patch
Correcting my previous comment - the test that this patch fixes (on Chromium Win/Linux) are: canvas/philip/tests/2d.shadow.transform.2.html fast/canvas/canvas-scale-fillRect-shadow.html fast/canvas/canvas-scale-fillPath-shadow.html 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. Attachment 76161 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6911027 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? 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? Dunno. Mihaip recently changed how the EWS bot updates its DEPS. (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 on attachment 76161 [details]
Patch
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.
Created attachment 76486 [details]
Patch
Created attachment 76491 [details]
Patch
Comment on attachment 76491 [details]
Patch
Original patch no longer applies - updated so it does.
Comment on attachment 76491 [details] Patch 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 on attachment 76491 [details] Patch Clearing flags on attachment: 76491 Committed r74001: <http://trac.webkit.org/changeset/74001> All reviewed patches have been landed. Closing bug. |