[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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114275|review?                     |review-
               Flag|                            |




--- Comment #6 from Nikolas Zimmermann <zimmermann at kde.org>  2011-11-10 08:45:37 PST ---
(From update of attachment 114275)
View in context: https://bugs.webkit.org/attachment.cgi?id=114275&action=review

First round of review, thanks for tackling this!

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:57
> +    SVGRenderSupport::setNeedsLayoutForFilteredContainer(this);

I'd rename this to "setContainerNeedsLayoutIfNeeded".

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

Use early return here.

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

And here, for any of the cases.

> 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?

> LayoutTests/svg/repaint/filter-child-repaint.svg:13
> +      <feOffset in="SourceGraphic" dx="0" dy="0" result="topCopy"/>
> +      <feGaussianBlur in="SourceAlpha" stdDeviation="2" result="shadow"/>
> +      <feOffset in="shadow" dx="3" dy="3" result="movedShadow"/>
> +      <feMerge>
> +        <feMergeNode in="movedShadow"/>
> +        <feMergeNode in="topCopy"/>
> +      </feMerge>
> +    </filter>

You could use the new feDropShadow primitive, would simplify the test.

> 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 :-)

> 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.

> LayoutTests/svg/repaint/filter-child-repaint.svg:18
> +    <rect width="64" height="64" style="fill:rgb(0,128,0)"/>
> +    <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.

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