[webkit-reviews] review denied: [Bug 53088] SVG: "filter" race condition may prevent SVG elements from being re-drawn : [Attachment 119015] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Branimir Lambov
<blambov at google.com>'s request for review:
Bug 53088: SVG: "filter" race condition may prevent SVG elements from being
re-drawn
https://bugs.webkit.org/show_bug.cgi?id=53088

Attachment 119015: Patch
https://bugs.webkit.org/attachment.cgi?id=119015&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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);