[Webkit-unassigned] [Bug 52200] Small filter primitive renderer improvements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52200


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #79273|review?                     |review-
               Flag|                            |




--- Comment #16 from Dirk Schulze <krit at webkit.org>  2011-01-19 01:25:11 PST ---
(From update of attachment 79273)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.