Bug 54451 – FESpecularLightingElement changes doesn't require relayout.

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

FESpecularLightingElement changes doesn't require relayout.

[Renata Hodovan]

Created attachment 82443 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 82443 [details]
Patch

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

Some questions from my side, leaving r? until we resolved it on IRC.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:103
> +    LightSource* lightSource = const_cast<LightSource*>(specularLighting->lightSource());
> +    const SVGFELightElement* lightElement = findLightElement();

This is guaranteed to be non null?
Please add assertions.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:149
> +    // The light element has different attribute names.

I don't get this comment.

> Source/WebCore/svg/SVGFESpecularLightingElement.h:36
>      static PassRefPtr<SVGFESpecularLightingElement> create(const QualifiedName&, Document*);
> +    void lightElementAttributeChanged(const SVGFELightElement*, const QualifiedName&);

Who is using this method?

> Source/WebCore/svg/SVGFESpecularLightingElement.h:42
> +    virtual bool setFilterEffectAttribute(FilterEffect*, const QualifiedName&);

Ditto?

[Renata Hodovan]

(In reply to comment #2)
> (From update of attachment 82443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82443&action=review
> 
> Some questions from my side, leaving r? until we resolved it on IRC.
> 
> > Source/WebCore/svg/SVGFESpecularLightingElement.cpp:103
> > +    LightSource* lightSource = const_cast<LightSource*>(specularLighting->lightSource());
> > +    const SVGFELightElement* lightElement = findLightElement();
> 
> This is guaranteed to be non null?
> Please add assertions.
All right, an assertion would be really useful.

> > Source/WebCore/svg/SVGFESpecularLightingElement.cpp:149
> > +    // The light element has different attribute names.
> 
> I don't get this comment.
I wanted to say since the attr names of light element are different they are unambiguously identifiable.

> > Source/WebCore/svg/SVGFESpecularLightingElement.h:36
> >      static PassRefPtr<SVGFESpecularLightingElement> create(const QualifiedName&, Document*);
> > +    void lightElementAttributeChanged(const SVGFELightElement*, const QualifiedName&);
> 
> Who is using this method?
> 
> > Source/WebCore/svg/SVGFESpecularLightingElement.h:42
> > +    virtual bool setFilterEffectAttribute(FilterEffect*, const QualifiedName&);
> 
> Ditto?
lightElementAttributeChanged is called from the light source. Its purpose is pass the arguments to the renderer of SpecularLighting. setFilterEffectAttribute sets the attribute of the FilterEffect object, called by the renderer.

[Renata Hodovan]

Created attachment 82784 [details]
Patch

[Dirk Schulze]

Comment on attachment 82784 [details]
Patch

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

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:218
> +SVGFELightElement* SVGFESpecularLightingElement::findLightElement() const
> +{
> +    for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +        if (node->hasTagName(SVGNames::feDistantLightTag)
> +            || node->hasTagName(SVGNames::fePointLightTag)
> +            || node->hasTagName(SVGNames::feSpotLightTag)) {
> +            return static_cast<SVGFELightElement*>(node);
> +        }
> +    }
> +    return 0;
> +}
> +
> +PassRefPtr<LightSource> SVGFESpecularLightingElement::findLight() const
> +{
> +    SVGFELightElement* lightNode = findLightElement();
> +    if (!lightNode)
> +        return 0;
> +    return lightNode->lightSource();
> +}

We have the same functions in SVGFEDiffuseLightningElement.cpp can we share them?

[Zoltan Herczeg]

> We have the same functions in SVGFEDiffuseLightningElement.cpp can we share them?

There is historical reasons here. I always wondered why we don't have a common base class for the lighting filters. A separate patch could do this improvement.

[Dirk Schulze]

(In reply to comment #6)
> > We have the same functions in SVGFEDiffuseLightningElement.cpp can we share them?
> 
> There is historical reasons here. I always wondered why we don't have a common base class for the lighting filters. A separate patch could do this improvement.

True, but why not put it into a static public function in SVGFELightElement for now?

[Zoltan Herczeg]

> True, but why not put it into a static public function in SVGFELightElement for now?

Are you sure? If they don't have a common ancestor, you cannot put them into a static function (since the offset of the member is different), maybe a template function can do that, but that would make things more compilcated...

[Dirk Schulze]

(In reply to comment #8)
> > True, but why not put it into a static public function in SVGFELightElement for now?
> 
> Are you sure? If they don't have a common ancestor, you cannot put them into a static function (since the offset of the member is different), maybe a template function can do that, but that would make things more compilcated...

Hm, what do you mean with different offset? a specularLighting or diffuseLigthning element should just have the 3 LightElements as child. So the structure is the same? 
I'd combine the two methods in a function:

PassRefPtr<LightSource> SVGFELightElement::findLight(const SVGElement* svgElement) const
{
   ASSERT(svgElement->hasTagName(SVGNames::feDiffuseLigthningTag) || svgElement->hasTagName(SVGNames::feSpecularLigthningTag)

  for (Node* node = svgElement->firstChild(); node; node = node->nextSibling()) {
  ...
}

Isn't this possible?

[Zoltan Herczeg]

> Isn't this possible?

Looks like ok. And move it into the LightSource base class is a good idea as well.

[Renata Hodovan]

Created attachment 82949 [details]
Patch

> Hm, what do you mean with different offset? a specularLighting or diffuseLigthning element should just have the 3 LightElements as child. So the structure is the same? 
> I'd combine the two methods in a function:
The common functions are moved into the LightElement class, but i couldn't merge the two methods 'cause there are used different places without reference to each other.

[Dirk Schulze]

Comment on attachment 82949 [details]
Patch

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

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:195
> +    RefPtr<FilterEffect> effect = FESpecularLighting::create(filter, color, surfaceScale(), specularConstant(),
> +                                          specularExponent(), kernelUnitLengthX(), kernelUnitLengthY(), SVGFELightElement::findLight(this));

I'm unsure about the SVG specification. If a feDiffuseLighting element (same for specular ligtning) doesn't have a light source, should we create the filter effect at all? Is it still valid? Or do we need an early return if !findLight ?

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:146
> +    if (SVGFELightElement::findLightElement(this) != lightElement)
>          return;

Does it mean, we don't propagate the changed attribute, or do we also skip the complete primitive on drawing? What is expected here, for lightElement == 0?

[Dirk Schulze]

(In reply to comment #12)
I should have asked zherzeg on his patch before. Sorry for not doing it. :-( We might also need a test where we remove the light source after the first drawing.

[Renata Hodovan]

(In reply to comment #12)
> (From update of attachment 82949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82949&action=review
> 
> > Source/WebCore/svg/SVGFESpecularLightingElement.cpp:195
> > +    RefPtr<FilterEffect> effect = FESpecularLighting::create(filter, color, surfaceScale(), specularConstant(),
> > +                                          specularExponent(), kernelUnitLengthX(), kernelUnitLengthY(), SVGFELightElement::findLight(this));
> 
> I'm unsure about the SVG specification. If a feDiffuseLighting element (same for specular ligtning) doesn't have a light source, should we create the filter effect at all? Is it still valid? Or do we need an early return if !findLight ?
from the spec of feSpecularLight: "Content model: Any number of descriptive elements and _exactly one_ light source element, in any order." so i think it should be invalid without any light source

> > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:146
> > +    if (SVGFELightElement::findLightElement(this) != lightElement)
> >          return;
> 
> Does it mean, we don't propagate the changed attribute, or do we also skip the complete primitive on drawing? What is expected here, for lightElement == 0?
It is a valid construction if we add multiple light source to the diffuse/specular light node, but only the first one should be used by the filter primitive, the others should be ignored, including their attribute changes. lightElement cannot be 0 since the light element calls this by passing "this" to the argument. If this would be NULL, that would be a serious error

[Dirk Schulze]

(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 82949 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82949&action=review
> > 
> > > Source/WebCore/svg/SVGFESpecularLightingElement.cpp:195
> > > +    RefPtr<FilterEffect> effect = FESpecularLighting::create(filter, color, surfaceScale(), specularConstant(),
> > > +                                          specularExponent(), kernelUnitLengthX(), kernelUnitLengthY(), SVGFELightElement::findLight(this));
> > 
> > I'm unsure about the SVG specification. If a feDiffuseLighting element (same for specular ligtning) doesn't have a light source, should we create the filter effect at all? Is it still valid? Or do we need an early return if !findLight ?
> from the spec of feSpecularLight: "Content model: Any number of descriptive elements and _exactly one_ light source element, in any order." so i think it should be invalid without any light source
> 
Wrote you on IRC. Please return earlier, if no light element was found.

> > > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:146
> > > +    if (SVGFELightElement::findLightElement(this) != lightElement)
> > >          return;
> > 
> > Does it mean, we don't propagate the changed attribute, or do we also skip the complete primitive on drawing? What is expected here, for lightElement == 0?
> It is a valid construction if we add multiple light source to the diffuse/specular light node, but only the first one should be used by the filter primitive, the others should be ignored, including their attribute changes. lightElement cannot be 0 since the light element calls this by passing "this" to the argument. If this would be NULL, that would be a serious error

So we can turn this to an assert?

[Renata Hodovan]

Created attachment 83291 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 83291 [details]
Patch

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

Looks good, r=me with some comments, to be fixed before landing:

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:261
> +

Unrelated change, please remove.

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:145
> +    if (SVGFELightElement::findLightElement(this) != lightElement)

When does this happen, again? I keep forgetting.

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:202
> +

Remove this empty newline.

> Source/WebCore/svg/SVGFELightElement.h:36
> +    static PassRefPtr<LightSource> findLight(const SVGElement*);

findLightSource sounds better, please rename.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:209
> +

Empty newline, please remove.

[Renata Hodovan]

> > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:145
> > +    if (SVGFELightElement::findLightElement(this) != lightElement)
> 
> When does this happen, again? I keep forgetting.
For example, imagine that we have a specularLight with more than one lightSources
<feSpecularLight>
    <feSpotLight>...</feSpotLight>
    <fePointLight>...</fePointLight>
</feSpecularLight>
Now if we try to change the pointLight source we don't need to do anything 'cause
only the first lightSource is relevant according to the spec.
Is it okay? Can i land it?

[Nikolas Zimmermann]

(In reply to comment #18)
> > > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:145
> > > +    if (SVGFELightElement::findLightElement(this) != lightElement)
> > 
> > When does this happen, again? I keep forgetting.
> For example, imagine that we have a specularLight with more than one lightSources
> <feSpecularLight>
>     <feSpotLight>...</feSpotLight>
>     <fePointLight>...</fePointLight>
> </feSpecularLight>
> Now if we try to change the pointLight source we don't need to do anything 'cause
> only the first lightSource is relevant according to the spec.
> Is it okay? Can i land it?
Ah, perfectly fine. Sure go ahead, you already had r+, that shouldn't have blocked you :-)

[Renata Hodovan]

Committed r79674: <http://trac.webkit.org/changeset/79674>