FESpecularLightingElement changes doesn't require relayout.
Created attachment 82443 [details] Patch
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?
(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.
Created attachment 82784 [details] Patch
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?
> 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.
(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?
> 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...
(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?
> Isn't this possible? Looks like ok. And move it into the LightSource base class is a good idea as well.
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 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?
(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.
(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
(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?
Created attachment 83291 [details] Patch
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.
> > 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?
(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 :-)
Committed r79674: <http://trac.webkit.org/changeset/79674>