[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 #119015|review?                     |review-
               Flag|                            |




--- Comment #10 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-14 01:53:55 PST ---
(From update of attachment 119015)
View in context: https://bugs.webkit.org/attachment.cgi?id=119015&action=review

Great work! I have some concerns, especially regarding performance, as you walk the parent hierachy twice in certain cases. See my comments:

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:162
> +        RenderStyle* style = object->style();
> +        ASSERT(style);
> +
> +        const SVGRenderStyle* svgStyle = style->svgStyle();
> +        ASSERT(svgStyle);

I think we can safely assume, that these are available, SVG always assumed they aren't, but none of the other rendering/ code does (functions like containingBlock() etc, always access m_style w/o null checks).
Just remove those lines completly, except the ASSERT(object);

I'm not sure that checking hasFilter() actually still makes sense, this check dates back to a time where we didn't have a SVGResourcesCache, and had to lookup the filter right from the style.
Today, we just query the SVGResourcesCache, so you can safely remove the hasFilter() check, and avoid the asserts and local variables.

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

if (!resources || !resources->filter()) reads better.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:200
> +    markFiltersForInvalidation(object);
> +    markParentsForInvalidation(object);

This now walks the parent chine twice! We have to avoid that, you need to refactor the code further. eg. with a general markForFoobarMethod(), that takes an enum MarkMode { MarkFiltersForInvalidation = 1 << 0, MarkParentsForInvaldiation = 1 << 1}, so you can combine flags like MarkFilterForInvalidation | MarkParentsForInvalidation, or just use them individually. This way you can write one optimized method, that walks the parent hierachy once, and does the right job for each of the combinations.

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-127
>> -        resources->removeClientFromCache(object);
> 
> This is being called from layout(), which in turn will never be called if the filter is not already invalidated.

This information needs to go in the ChangeLog!

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-155
>> -        resources->removeClientFromCache(renderer);
> 
> This is not sufficient as any filter in the ascendants' chain will block updates to the element. Changed to invalidate all filters in the chain.

Same here.

> LayoutTests/svg/filters/filter-refresh.svg:115
> +    setTimeout(wait, 0);
> +    
> +    function wait() {
> +      // Single timeout doesn't seem to wait for the initial painting, wait again.
> +      setTimeout(change, 0);
> +    }

You have to listen for the window.onload = function() { .. } event. There you can call setTimeout(change, 0);
This <script> otherwise executes immediately, as its seen by the parser.

I'd highly propose to split up this testcase. Large testcases like this makes it hard to track errors and debug them later, once they fail.
I know this will be lots of work, but it'll really help us to get full coverage for all filter invalidation combinations, with nice self-contained testcases, for all kinds of cases. nested containers, use, etc. as in your large all-in-one testcase now :-)

> LayoutTests/svg/filters/filter-refresh.svg:125
> +          setTimeout("layoutTestController.notifyDone()", 0);

You'd usually use anonymous functions here: setTimeout(function() { layoutTestController.notifyDone(); }, 0);

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