[Webkit-unassigned] [Bug 71733] Repaint broken when children of filtered SVG elements are updated

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Joel Webber <jgw at chromium.org>  2011-11-10 10:18:46 PST ---
(From update of attachment 114275)
View in context: https://bugs.webkit.org/attachment.cgi?id=114275&action=review

>> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:57

> 
> I'd rename this to "setContainerNeedsLayoutIfNeeded".

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:310
>> +    if (object->normalChildNeedsLayout()) {
> 
> Use early return here.

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:312
>> +        if (resources && resources->filter())
> 
> And here, for any of the cases.

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:313
>> +            object->setNeedsLayout(true);
> 
> This is dangerous, you're mutating the setNeedsLayout state of all containing blocks as well, are you sure you want that? Doing that from within layout() that looks suspicious.
> I'd propose to amend RenderSVGContainers way of laying out the children, where we currently do:
>     SVGRenderSupport::layoutChildren(this, selfNeedsLayout());
> 
> This could just read layoutChildren(this, selfNeedsLayout() || filtersForceContainerLayout())
> where filtersForceContainerLayout() contains your object->normalChildNeedsLayout() && resources->filter() check.
> Sounds better, eh?

Thanks for clarifying that -- it felt a bit odd to be flipping that bit here, and I failed to notice the hierarchical behavior of calling setNeedsLayout(). I've refactored this code as you suggest, and it does indeed feel better.

>> LayoutTests/svg/repaint/filter-child-repaint.svg:13
>> +    </filter>
> 
> You could use the new feDropShadow primitive, would simplify the test.

Done, thanks. That one wasn't in the SVG 1.1 docs, but I see it in WebKit now.
(This awkward implementation, btw, came from a chromium bug I was investigating)

>> LayoutTests/svg/repaint/filter-child-repaint.svg:15
>> +
> 
> Can you add a <title> section, explaining when this tests passes?
> Currently its hard to tell, without tracing in mind :-)

Done. It's a bit hard to tell what the norms are from existing tests :)

>> LayoutTests/svg/repaint/filter-child-repaint.svg:16
>> +  <g filter='url(#dropShadow)' width='128px' height='128px'>
> 
> width/height are useless on <g>, you can remove it.

Done.

>> LayoutTests/svg/repaint/filter-child-repaint.svg:18
>> +    <rect id='poke' x='32' y='32' width="64" height="64" style="fill:rgb(128,0,0)"/>
> 
> Also you could just use fill="green" here.

Done (old habits die hard...).

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