[webkit-reviews] review denied: [Bug 99996] SVG Filter Updates do not trigger repaint : [Attachment 170378] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Dominik Röttsches (drott)
<dominik.rottsches at intel.com>'s request for review:
Bug 99996: SVG Filter Updates do not trigger repaint
https://bugs.webkit.org/show_bug.cgi?id=99996

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=170378&action=review


> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:54
> +    if (node()->hasTagName(SVGNames::feFloodTag) ||
node()->hasTagName(SVGNames::feDropShadowTag)) {

Looks reasonable. Wasn't that enough to trigger repainting?

> Source/WebCore/svg/SVGFEDropShadowElement.cpp:86
> +bool SVGFEDropShadowElement::setFilterEffectAttribute(FilterEffect* effect,
const QualifiedName& attrName)

It is very strange that this is implemented in some SVGFE*Elements, but not in
others.

> Source/WebCore/svg/SVGFEDropShadowElement.cpp:101
> +    if (attrName == SVGNames::flood_colorAttr) {
> +	   dropShadow->setShadowColor(style->svgStyle()->floodColor());
> +	   return true;
> +    }
> +    if (attrName == SVGNames::flood_opacityAttr) {
> +	   dropShadow->setShadowOpacity(style->svgStyle()->floodOpacity());
> +	   return true;
> +    }

This is very strange that we have that here (and in other elements). I think I
can remember why we added it. But it is still not the proper solution.
Repainting should be handled by RenderStyle on CSS Properties. While this may
work with attributes, what about inline styles?

feDropShadoe.style.floodColor = 0.5;

Does this repaint? Can you check that please?

>> Source/WebCore/svg/SVGFEDropShadowElement.cpp:158
>> +	    if (attrName != SVGNames::flood_colorAttr && attrName !=
SVGNames::flood_opacityAttr)
> 
> Do we need to do something with the attribute values locally?

This is handled by CSS. You can remove this.

>> Source/WebCore/svg/SVGFEDropShadowElement.cpp:-150
>> -	ASSERT_NOT_REACHED();
> 
> I removed this assertion since it's only testing whether isSupportedAttribute
is identical to those four.

Why remove it? So we make sure that we don't miss something.

> LayoutTests/ChangeLog:10
> +	   Rebaselining GTK and EFL svg filter tests after repainting issue has
been fixed.
> +	   For Chromium, Mac & Qt I am marking the tests as ImageOnlyFailure
since pixel results on these ports
> +	   need updates.

Looking at your tests, I realized two things. First, some repaint areas are
huge! Why is that the case? Second, CSS properties don't trigger repainting.
That means the whole concept is wrong and needs to be removed. This is not your
fault, but making the attribute changes working is not the right approach. We
need to refactor the whole thing and let RenderStyle do it's job.