[webkit-reviews] review denied: [Bug 52200] Small filter primitive renderer improvements : [Attachment 79273] patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 52200: Small filter primitive renderer improvements
https://bugs.webkit.org/show_bug.cgi?id=52200

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

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

> Source/WebCore/ChangeLog:11
> +	   calls relayout (Thus, this is just a performance overhead
> +	   at this moment).

Please make a detailed description how you plan to reach the goal and what
you're doing right now: What happens if an attribute change? What does the
renderer of the primitive element do?

> Source/WebCore/ChangeLog:42
> +	   Demoing the new attribute setting mechanism.

Demonstration of ...

> Source/WebCore/ChangeLog:49
> +	   Scanning the FilterBuilder list, and calls the necessary callbacks.

s/calls/call/

> Source/WebCore/ChangeLog:57
> +	   FilterEffect belongs to this RenderObject.

that belongs or maybe belonging

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:334
> +	   HashSet<FilterEffect*>::iterator end = effectReferences.end();
> +	   for (HashSet<FilterEffect*>::iterator it = effectReferences.begin();
it != end; ++it)

In other SVG files, we often use 
HashSet<FilterEffect*>::iterator it  = effectReferences.begin();
HashSet<FilterEffect*>::iterator end = effectReferences.end();
for (; it != end; ++it)

but it does not really matter.

Makes it really sense to clear all effects this way? I mean this function just
makes sense if you want to clear _all_ effects. But isn't it already covered by
calling the DTor of FilterBuilder?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:339
> +void RenderSVGResourceFilter::setAllFilterEffectsAttribute(RenderObject*
object, const QualifiedName& attribute)

s/setAllFilterEffectsAttribute/setAllFilterEffectAttributes/

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:351
> +	   if (effect) {
> +	       primitve->setFilterEffectAttribute(effect, attribute);
> +	       recursiveClearResult(builder, effect);
> +	   }
> +    }

if (!effect)
   continue;

...

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:40
> -    explicit
RenderSVGResourceFilterPrimitive(SVGFilterPrimitiveStandardAttributes*
filterPrimitiveElement)
> +    explicit RenderSVGResourceFilterPrimitive(SVGStyledElement*
filterPrimitiveElement)

Why did you change the elements type?

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:108
> +    if (attrName == SVGNames::diffuseConstantAttr
> +	   || attrName == SVGNames::surfaceScaleAttr
> +	   || attrName == SVGNames::lighting_colorAttr)
> +	   setAllFilterEffectsAttribute(attrName);
> +

This should be in a separate patch, together with dynamic update tests.

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:76
> +    // When all filters support this method, it will be changed to a pure
virtual method.
> +    ASSERT_NOT_REACHED();

If not all filters implemented this function, don't we run into this assert?

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:61
> +    inline void setAllFilterEffectsAttribute(const QualifiedName& attribute)


arh. setFilterEffectAttribute and setAllFilterEffectsAttribute sounds to
simular for me. I even don't see a difference in the behavior if I just look at
the names. setAttributeToAllFilterEffects? Hm. even that sounds a bit strange.
Of course you should rename the function in the renderer as well.