[Webkit-unassigned] [Bug 53088] SVG: "filter" race condition may prevent SVG elements from being re-drawn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53088


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #118789|1                           |0
        is obsolete|                            |




--- Comment #7 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-12 09:17:07 PST ---
(From update of attachment 118789)
View in context: https://bugs.webkit.org/attachment.cgi?id=118789&action=review

Nice patch, but with some risks. r- for the test problem and my questions regarding the approach.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:153
> +#if ENABLE(FILTERS)

This won't build on release builds. You need to use UNUSED_PARAM(object) in the #else branch.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:162
> +    if (object->style()->svgStyle()->hasFilter()) {

You could early return here, if there's no filter. Its the preferred way in WebKit.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:164
> +        if (resources && resources->filter())

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:177
> +    // Invalidate filters as they will hold cached copies of their children.
> +    markFiltersForInvalidation(object);

Hm, we have various other places with special cased filter code - can you check all of them, and tell us whether your "sledgehammer" approach (which is probably correct) potentially makes them unnecessary.
It's been a while since I checked the code the last time, but I'll bet grepp'ing for resources->filter() or sth. similar will show you several places with invalidation special cases for filters.

I just want be sure that we don't add new code, as general as yours, that supersedes existing incorrect special cases.

> LayoutTests/svg/filters/filter-refresh.svg:125
> +    // eventSender.leapForward(100) does not seem to trigger the animation, so we wait here.
> +    setTimeout(change, 60);

This is always a bit dangerous, potentially leading to races.
I'd rather start the animations manually from script, by using begin="indefinite" everywhere, and calling foo.beginElement() for all animations, at once.
As you're using <set> animations, you can then set a 0ms timer, if that fires, the animation should have ran. But this should only be done if you _need_ SMIL animations to trigger the bug.
We don't support begin/endEvent handling yet, so you cannot reliable know when the SMIL animation finished.

I guess you don't want SMIL animations at all so I'd like to see a variant of this test without any <set> elements at all, but rather with scripted animations from JS. That would be safer.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.