Bug 54451 - FESpecularLightingElement changes doesn't require relayout.
Summary: FESpecularLightingElement changes doesn't require relayout.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Renata Hodovan
URL:
Keywords:
Depends on:
Blocks: 55075
  Show dependency treegraph
 
Reported: 2011-02-15 05:19 PST by Renata Hodovan
Modified: 2011-02-25 05:00 PST (History)
3 users (show)

See Also:


Attachments
Patch (11.94 KB, patch)
2011-02-15 06:08 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (14.22 KB, patch)
2011-02-17 05:16 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (16.83 KB, patch)
2011-02-18 06:06 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (73.17 KB, patch)
2011-02-22 02:51 PST, Renata Hodovan
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2011-02-15 05:19:20 PST
FESpecularLightingElement changes doesn't require relayout.
Comment 1 Renata Hodovan 2011-02-15 06:08:15 PST
Created attachment 82443 [details]
Patch
Comment 2 Nikolas Zimmermann 2011-02-17 02:38:04 PST
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?
Comment 3 Renata Hodovan 2011-02-17 05:16:03 PST
(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.
Comment 4 Renata Hodovan 2011-02-17 05:16:47 PST
Created attachment 82784 [details]
Patch
Comment 5 Dirk Schulze 2011-02-18 03:44:10 PST
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?
Comment 6 Zoltan Herczeg 2011-02-18 04:19:49 PST
> 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.
Comment 7 Dirk Schulze 2011-02-18 04:28:31 PST
(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?
Comment 8 Zoltan Herczeg 2011-02-18 04:33:54 PST
> 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...
Comment 9 Dirk Schulze 2011-02-18 04:53:04 PST
(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?
Comment 10 Zoltan Herczeg 2011-02-18 05:01:59 PST
> Isn't this possible?

Looks like ok. And move it into the LightSource base class is a good idea as well.
Comment 11 Renata Hodovan 2011-02-18 06:06:48 PST
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.
Comment 12 Dirk Schulze 2011-02-18 06:24:10 PST
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?
Comment 13 Dirk Schulze 2011-02-18 06:25:57 PST
(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.
Comment 14 Renata Hodovan 2011-02-18 06:40:37 PST
(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
Comment 15 Dirk Schulze 2011-02-18 06:45:44 PST
(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?
Comment 16 Renata Hodovan 2011-02-22 02:51:34 PST
Created attachment 83291 [details]
Patch
Comment 17 Nikolas Zimmermann 2011-02-23 13:06:19 PST
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.
Comment 18 Renata Hodovan 2011-02-24 04:04:14 PST
> > 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?
Comment 19 Nikolas Zimmermann 2011-02-25 01:50:31 PST
(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 :-)
Comment 20 Renata Hodovan 2011-02-25 05:00:20 PST
Committed r79674: <http://trac.webkit.org/changeset/79674>