Bug 71731

<rdar://problem/10408178>

Created attachment 113926 [details] screenshot of webkit vs opera

Created attachment 113927 [details] repro

This is affecting the SVG Wow! Ripple demo.

This might be the root cause of https://bugs.webkit.org/show_bug.cgi?id=68664 (or, rather, they might be the same bug).

I'll take this one, it looks straightforward. Something to do with the mechanics surrounding SVGFEImageElement's m_cachedImage.

(In reply to comment #6) > I'll take this one, it looks straightforward. Something to do with the mechanics surrounding SVGFEImageElement's m_cachedImage. Hey Tim, I'm also working on feImage as we speak, but found a much larger problem when using feImage with primitiveUnits="objectBoundingBox" and referencing other elements by #elementName. I can explain it later, once I come to work.. Cheers, Niko

We're trying to resolve the href too early; we should do it on the first build() instead of when the attribute's set, I think. There are other issues here, with invalidation and the like, but those will be addressed in another bug. This also exposes a crash when the referenced element is 0x0 pixels, so I've fixed that as well.

Created attachment 114390 [details] simple patch

Comment on attachment 114390 [details] simple patch View in context: https://bugs.webkit.org/attachment.cgi?id=114390&action=review > LayoutTests/svg/filters/feImage-reference-svg-primitive.svg:11 > + <circle id="circle" r="100" fill="red" /> Red should not be visible in passing pixel results.

(In reply to comment #10) > (From update of attachment 114390 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114390&action=review > > > LayoutTests/svg/filters/feImage-reference-svg-primitive.svg:11 > > + <circle id="circle" r="100" fill="red" /> > > Red should not be visible in passing pixel results. Thanks, Simon! Landed as r99782.

Reopened because of rollout.

The test failures only occur in DRT; in Safari, everything looks fine.

Created attachment 117088 [details] patch which works in DRT

Comment on attachment 117088 [details] patch which works in DRT I think this is a good patch overall. Some notes: View in context: https://bugs.webkit.org/attachment.cgi?id=117088&action=review > LayoutTests/ChangeLog:22 > + * platform/mac/svg/filters/feImage-reference-invalidation-expected.png: Added. > + * platform/mac/svg/filters/feImage-reference-invalidation-expected.txt: Added. > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.png: Added. > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.txt: Added. These test cases seems rather simple, can't we use the same results on all platforms? > LayoutTests/svg/filters/feImage-reference-invalidation.svg:6 > + <rect id="circle" width="100" height="100" fill="red" /> > + <rect id="circle2" width="100" height="100" fill="green" /> Why this is a rect if its name is circle?

Comment on attachment 117088 [details] patch which works in DRT Attachment 117088 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10693183 New failing tests: svg/filters/feImage-reference-invalidation.svg svg/filters/feImage-reference-svg-primitive.svg svg/filters/feImage-zero-size-crash.svg

Comment on attachment 117088 [details] patch which works in DRT View in context: https://bugs.webkit.org/attachment.cgi?id=117088&action=review > LayoutTests/ChangeLog:21 > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.png: Added. It's a shame that this passing result contains red.

Created attachment 117230 [details] patch fixing notes

(In reply to comment #15) > (From update of attachment 117088 [details]) > I think this is a good patch overall. Some notes: > > View in context: https://bugs.webkit.org/attachment.cgi?id=117088&action=review > > > LayoutTests/ChangeLog:22 > > + * platform/mac/svg/filters/feImage-reference-invalidation-expected.png: Added. > > + * platform/mac/svg/filters/feImage-reference-invalidation-expected.txt: Added. > > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.png: Added. > > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.txt: Added. > > These test cases seems rather simple, can't we use the same results on all platforms? In theory? Yes. In practice, I've (almost) never made pixel results that work on the Chromium bot also. I've just uploaded a new patch with non-platform-specific results (I just remembered I forgot to update the ChangeLog, I'll do that locally) just to see if the cr-linux bot accepts my results, otherwise I'll land it with the platform-specific results.

(In reply to comment #19) > (In reply to comment #15) > > (From update of attachment 117088 [details] [details]) > > I think this is a good patch overall. Some notes: > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=117088&action=review > > > > > LayoutTests/ChangeLog:22 > > > + * platform/mac/svg/filters/feImage-reference-invalidation-expected.png: Added. > > > + * platform/mac/svg/filters/feImage-reference-invalidation-expected.txt: Added. > > > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.png: Added. > > > + * platform/mac/svg/filters/feImage-reference-svg-primitive-expected.txt: Added. > > > > These test cases seems rather simple, can't we use the same results on all platforms? > > In theory? Yes. In practice, I've (almost) never made pixel results that work on the Chromium bot also. I've just uploaded a new patch with non-platform-specific results (I just remembered I forgot to update the ChangeLog, I'll do that locally) just to see if the cr-linux bot accepts my results, otherwise I'll land it with the platform-specific results. Actually, that doesn't look like a winning approach today (the cr-linux EWS machine seems stuck), so I'll commit given Simon's r+ above and keep an eye on the bots.

Landed (again) in r101542.