[webkit-reviews] review denied: [Bug 71733] Repaint broken when children of filtered SVG elements are updated : [Attachment 114275] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Joel Webber
<jgw at chromium.org>'s request for review:
Bug 71733: Repaint broken when children of filtered SVG elements are updated
https://bugs.webkit.org/show_bug.cgi?id=71733

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

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