CSS related SVG*Element changes doesn't require relayout

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

CSS related SVG*Element changes doesn't require relayout

[Renata Hodovan]

Created attachment 86589 [details]
Proposed patch

[Dirk Schulze]

Comment on attachment 86589 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86589&action=review

I think you miss the case, where a CSS property gets changed on a filter element

<filter id="f1" style="flood-color:red">
<feFlood flood-color="inherit"/>
</filter>

I think we should invalidate in this case, since we don't know which effect is affected by this change and therefor we can't update the FE*.cpp. Just remove renderer->isSVGResourceFilter() in SVGResourceCache. I'd still  like to see a test case with this scenario.

> Source/WebCore/ChangeLog:10
> +        we need here an early return. So RenderSVGResourceFilterPrimitive::styleDidChange() can handle these properties

s/here//

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:42
> +        return;

What should happen for this case? At the moment we would neither repaint, nor relayout, right?

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:129
> +    // In this case the proper SVGFE*Element will deceide whether the modifided CSS properties implies a relayout or repaint.

s/deceide/decide/

[Renata Hodovan]

> I think you miss the case, where a CSS property gets changed on a filter element
> 
> <filter id="f1" style="flood-color:red">
> <feFlood flood-color="inherit"/>
> </filter>
You are right. Should I add such tests for every CSS update? (I think of the two lighting filters)

> What should happen for this case? At the moment we would neither repaint, nor relayout, right?
Yeah, in this case doesn't happen anything. But I think it's just a sanity check because the control flow can only reach this point if the given object is a filter.

[Nikolas Zimmermann]

sorry, can't comment in detail. Just want to say, that using styleForRenderer() is an absolute hack, this should be avoided! Now that we have renderers that cache the style, doing any manual style computation is not needed anymore. Will go more into detail tomorrow.

[Renata Hodovan]

(In reply to comment #4)
> sorry, can't comment in detail. Just want to say, that using styleForRenderer() is an absolute hack, this should be avoided! Now that we have renderers that cache the style, doing any manual style computation is not needed anymore. Will go more into detail tomorrow.
I see what you want to say and i guess SVGFE*Element::build() functions also should follow this conception instead of using styleForRenderer().

[Renata Hodovan]

Created attachment 86946 [details]
Proposed patch

[Dirk Schulze]

Comment on attachment 86946 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86946&action=review

> Source/WebCore/ChangeLog:8
> +        The changes of some CSS related SVGFilter properties e.g. lighting-color, flood_color, flood_opacity

s/_/-/

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:92
> +    if (attrName == QualifiedName(nullAtom, nullAtom, nullAtom))

Isn't it enough to check if localName() is a nullatom? Also, don't we have something like a nullQualifiedName? Some kind of static variable?

> Source/WebCore/svg/SVGFEFloodElement.cpp:46
> +    if (attrName == QualifiedName(nullAtom, nullAtom, nullAtom)) {

Ditto.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:97
> +    if (attrName == QualifiedName(nullAtom, nullAtom, nullAtom))

Ditto.

> LayoutTests/ChangeLog:9
> +        Testing inherited CSS related SVG property changes by FEFlood, FESpecularLighting and FEDiffuseLighing filters.
> +        Adding a missing test to check the dynamic update of lighting-color property of FESpecularLighting.

Can you describe why the pixel tests don't pass?
Also SVGFESpecularLightingElement-inherit-lighting-color-css-prop-expected.png is completely whit. More confusing :-)

[Nikolas Zimmermann]

Comment on attachment 86946 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86946&action=review

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:38
> +void RenderSVGResourceFilterPrimitive::styleDidChange(StyleDifference diff, const RenderStyle* style)

You should take StyleDifference into account. Eg. If it is StyleDifferenceEqual you don't have to call primitiveAttributeChanged.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> +    static_cast<RenderSVGResourceFilter*>(filter)->primitiveAttributeChanged(this, WebCore::QualifiedName(nullAtom, nullAtom, nullAtom));

Hm, I don't like that approach, we should really figure out the attribute name here. For example if I set "stop-color" property on a FEDiffuseLighting element, then your code would assume that the lighting-color has changed.
That's obviously not correct.

You probably want to override styleWillChange as well, you can find out there whether eg. lighting-color changed, by comparing the oldStyles lightingColor with the newStyle.
Basically we want a method that compares two styles, and if lighting-color changes, it returns lightingColorAttr etc..

Can you follow me?

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:129
> +    // In this case the proper SVGFE*Element will decide whether the modifided CSS properties implies a relayout or repaint.

typo: modified.

> LayoutTests/svg/dynamic-updates/script-tests/SVGFESpecularLightingElement-lighting-color-css-prop.js:69
> +// [Name] SVGFESpecularLightingElement-lighting-color-css-prop.js

This seems doubled??

[Renata Hodovan]

> Can you describe why the pixel tests don't pass?
My bad... they pass I just messed up the expecteds :)

> You should take StyleDifference into account. Eg. If it is StyleDifferenceEqual you don't have to call primitiveAttributeChanged.
okay

> > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> > +    static_cast<RenderSVGResourceFilter*>(filter)->primitiveAttributeChanged(this, WebCore::QualifiedName(nullAtom, nullAtom, nullAtom));
> 
> Hm, I don't like that approach, we should really figure out the attribute name here. For example if I set "stop-color" property on a FEDiffuseLighting element, then your code would assume that the lighting-color has changed.
> That's obviously not correct.
> 
> You probably want to override styleWillChange as well, you can find out there whether eg. lighting-color changed, by comparing the oldStyles lightingColor with the newStyle.
> Basically we want a method that compares two styles, and if lighting-color changes, it returns lightingColorAttr etc..
> 
> Can you follow me?
Yeah, I understand you. But one detail is not clear to me: how could the calculated Attribute get from styleWillChange to styleDidChange?
I have a draft already which contains a new function in RenderSVGResourceFilterPrimitive. This function do the mentioned comparison and returns with the modified attribute. Do you really think I have to move it into styleWillChange?

[Renata Hodovan]

> Yeah, I understand you. But one detail is not clear to me: how could the calculated Attribute get from styleWillChange to styleDidChange?
> I have a draft already which contains a new function in RenderSVGResourceFilterPrimitive. This function do the mentioned comparison and returns with the modified attribute.
It raises a other question.... shouldn't we implement this comparison function into SVGStyledElement? We need such procedure by every CSS prop changes after all.

[Renata Hodovan]

Created attachment 87480 [details]
Proposed patch

[Dirk Schulze]

Comment on attachment 87480 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87480&action=review

> Source/WebCore/ChangeLog:8
> +        The changes of some CSS related SVGFilter properties e.g. lighting-color, flood_color, flood_opacity

Still forgot to change the underscores to -

> Source/WebCore/ChangeLog:11
> +        via RenderSVGResourceFilter::primitiveAttributeChanged() the same way like we do it by the other SVGAttributes.

s/by the other/for the other/

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:46
> +    if (diff == StyleDifferenceEqual)
> +        return;

this is uneccessary since SVGResourceCache::clientStyleChanged checks it. But you can create a ASSERT here if you want, but seems not needed.

> Source/WebCore/svg/SVGStyledElement.cpp:213
> +void SVGStyledElement::changedCSSProperties(Vector<QualifiedName>& result, const RenderStyle* oldStyle, const RenderStyle* newStyle)
> +{
> +    if (oldStyle->svgStyle()->floodColor() != newStyle->svgStyle()->floodColor())
> +        result.append(SVGNames::flood_colorAttr);
> +    if (oldStyle->svgStyle()->floodOpacity() != newStyle->svgStyle()->floodOpacity())
> +        result.append(SVGNames::flood_opacityAttr);
> +    if (oldStyle->svgStyle()->lightingColor() != newStyle->svgStyle()->lightingColor())
> +        result.append(SVGNames::lighting_colorAttr);
> +}

Discussing this on IRC. I wouldn't move it to SVGStyledElement. It's just the question which properties we will need as well. E.g. on filter we have at least one property, where we might not want to relayout, but which is not SVG specific: 'color'. I ask my self if we should move this function to RenderStyle and SVGRenderStyle. We need to know which properties are affected as well. Because just Filter seems to need this information, it should be part of RenderSVGResourceFilterPrimitive for now. We can move it to RenderStyle if we need it later.

[Dirk Schulze]

Comment on attachment 87480 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87480&action=review

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:131
> +    if (renderer->isSVGResourceFilterPrimitive())
> +        return;

Just noticed, that we would call invalidate(), if we change an unneeded property:

<feBlend flood-color="red" .../>
blend.style.floodColor = 'green';

flood-color doesn't have any affect for blend.

I'd take if (renderer->isSVGResourceFilterPrimitive() && diff == StyleDifferenceRepaint) return here. And ignore any invalidation in SVGFilterPrimitiveAttributeElement:svgAttributeChange if the attribute is  unknown. What do you think? We wouldn't repaint or relayout.

[Renata Hodovan]

*** Bug 54391 has been marked as a duplicate of this bug. ***

[Renata Hodovan]

> > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:46
> > +    if (diff == StyleDifferenceEqual)
> > +        return;
> 
> this is uneccessary since SVGResourceCache::clientStyleChanged checks it. But you can create a ASSERT here if you want, but seems not needed.

I think it'd be not so good, because SVGResourceCache::clientStyleChanged is called after this point. It's condition is just for avoiding the unwanted invalidation. But as you said on IRC we need to call the super class in every cases. So it'd better to call it on the top of RenderSVGResourceFilterPrimitive::styleDidChange(). This way we force an invalidation if it's necessary and later we check whether we need a repaint.

> I'd take if (renderer->isSVGResourceFilterPrimitive() && diff == StyleDifferenceRepaint) return here. And ignore any invalidation in SVGFilterPrimitiveAttributeElement:svgAttributeChange if the attribute is  unknown. What do you think? We wouldn't repaint or relayout.

Yes, you are right. We don't need neither invalidation nor repaint in this case.

[Renata Hodovan]

Created attachment 87680 [details]
Proposed patch

[Dirk Schulze]

Comment on attachment 87680 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87680&action=review

Patch looks great now. Just have a question before I can give a r+ and the other commands can be fixed on landing.

> Source/WebCore/ChangeLog:8
> +        The changes of some CSS related SVGFilter properties e.g. lighting_color, flood_color, flood_opacity

I said: "Still forgot to change the underscores to -", so the other way around. :-) You can fix it on landing.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> +    RenderSVGHiddenContainer::styleDidChange(diff, style);

This time you run styleDidChange before you run the code below, is this correct now? With the current code, SVGResourceCache::clientStyleChanged before updating the primitves, but it shouldn't be the problem here. Did you check this?

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:52
> +        static_cast<RenderSVGResourceFilter*>(filter)->primitiveAttributeChanged(this, SVGNames::flood_colorAttr);

just cast it once and store it in a variable before the if clause.

[Renata Hodovan]

> > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> > +    RenderSVGHiddenContainer::styleDidChange(diff, style);
> 
> This time you run styleDidChange before you run the code below, is this correct now? With the current code, SVGResourceCache::clientStyleChanged before updating the primitves, but it shouldn't be the problem here. Did you check this?

I tried out without 

7 if (diff == StyleDifferenceEqual)
 48 return; 

and I got a beautiful segfault because the style parameter was null :) So I thought we need this condition, which implies that the style isn't null. But it arrests us to reach the call of the super class...
I ran dynamic-update tests and there wasn't new failed test.

[Dirk Schulze]

Comment on attachment 87680 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87680&action=review

r=me  but with further comments to your LayoutTests, please fix the issues of my last comment as well as this one and you can land the patch. great work!

> LayoutTests/svg/dynamic-updates/script-tests/SVGFEDiffuseLightingElement-inherit-lighting-color-css-prop.js:17
> +var gradientElement = createSVGElement("feDiffuseLighting");

gradientElement should be feDiffuseLightingElement

> LayoutTests/svg/dynamic-updates/script-tests/SVGFESpecularLightingElement-inherit-lighting-color-css-prop.js:47
> +var rectElement = createSVGElement("image");

s/rectElement/imageElement/

> LayoutTests/svg/dynamic-updates/script-tests/SVGFESpecularLightingElement-lighting-color-css-prop.js:46
> +var rectElement = createSVGElement("image");

s/rectElement/imageElement/

[Renata Hodovan]

Landed in <http://trac.webkit.org/changeset/82565>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82565 might have broken Leopard Intel Debug (Tests)

[Dirk Schulze]

http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r82567%20(15931)/svg/

[Nikolas Zimmermann]

Comment on attachment 87680 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87680&action=review

This needs some tweaks to be correct:

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:41
> +void RenderSVGResourceFilterPrimitive::styleDidChange(StyleDifference diff, const RenderStyle* style)

Be careful, the 'style' parameter is actually 'oldStyle' and can be null!

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:46
> +    RenderObject* filter = parent();
> +    ASSERT(filter && filter->isSVGResourceFilter());

This doesn't work, styleDidChange() is called right after the renderer creation, before it is inserted in the render tree.
You need to null-check filter here.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:47
> +    if (diff == StyleDifferenceEqual)

That should read: if (diff == StyleDifferenceEqual || !oldStyle), once you renamed the second parameter of styleDidChange() from 'style' to 'oldStyle'.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:50
> +    const SVGRenderStyle* oldStyle = this->style()->svgStyle();

s/oldStyle/newStyle/!
It doesn't change the functionality, but it is confusing as it is right now.

[Renata Hodovan]

Last patch was rolled out. Uploading a new one...

[Renata Hodovan]

Created attachment 88929 [details]
Proposed patch

[Nikolas Zimmermann]

Comment on attachment 88929 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88929&action=review

r=me, some minor tweaks:

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:129
> +    // In this case the proper SVGFE*Element will deceide whether the modifided CSS properties implies a relayout or repaint.

typo: decide.
typo: imply,

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:92
> +        return diffuseLighting->setLightingColor(this->renderer()->style()->svgStyle()->lightingColor());

Is this->renderer() and this->renderer()->style() guaranteed to be non null here?
I'd like to see two asserts here.

> Source/WebCore/svg/SVGFEFloodElement.cpp:46
> +    RenderStyle* style = this->renderer()->style();

I'd like to see this->renderer*( stored in a local variable, and two assertions.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:97
> +    if (attrName == SVGNames::lighting_colorAttr)
> +        return specularLighting->setLightingColor(this->renderer()->style()->svgStyle()->lightingColor());

Ditto.

[Renata Hodovan]

Created attachment 89193 [details]
Proposed patch

I did your recommendations, but there was still one potential bug... Since we didn't verify that the object we got in styleDidChange was of the correct type, it could happen that after calling setFilterEffectAttribute() on another filter, we'd hit an ASSERT_NOT_REACHED. So I supplemented the patch with a type-check and this way it's safe already. :)

[Nikolas Zimmermann]

Comment on attachment 89193 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89193&action=review

Very nice, please do this before landing:

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:44
> +

How about moving the hasTagName check right here, after the base class call.
Then use early return style.

[Renata Hodovan]

> > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:44
> > +
> 
> How about moving the hasTagName check right here, after the base class call.
> Then use early return style.
Thanks, but are you sure I have to move it there? I have to do the comparison later as well, because we should separate the two cases (feFlood and lightings).

[Renata Hodovan]

Committed r83821: <http://trac.webkit.org/changeset/83821>