Bug 50437 – [skia] ignore transform for canvas shadows

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Lawther]

[skia] ignore transform for canvas shadows

[Mike Lawther]

Created attachment 75462 [details]
Patch

[Stephen White]

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.

[Mike Lawther]

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

[Mike Lawther]

Created attachment 76161 [details]
Patch

[Mike Lawther]

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.

[WebKit Review Bot]

Attachment 76161 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6911027

[Mike Lawther]

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?

[Mike Lawther]

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?

[Adam Barth]

Dunno.  Mihaip recently changed how the EWS bot updates its DEPS.

[Mihai Parparita]

(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.

[Eric Seidel (no email)]

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.

[Mike Lawther]

Created attachment 76486 [details]
Patch

[Mike Lawther]

Created attachment 76491 [details]
Patch

[Mike Lawther]

Comment on attachment 76491 [details]
Patch

Original patch no longer applies - updated so it does.

[Daniel Bates]

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).

[WebKit Review Bot]

Comment on attachment 76491 [details]
Patch

Clearing flags on attachment: 76491

Committed r74001: <http://trac.webkit.org/changeset/74001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.