[webkit-reviews] review denied: [Bug 53442] SVGAnimateElement needs information about the animated attribute type : [Attachment 80735] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 53442: SVGAnimateElement needs information about the animated attribute
type
https://bugs.webkit.org/show_bug.cgi?id=53442

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80735&action=review

Still some work todo:

> Source/WebCore/ChangeLog:8
> +	   For a type specific animation we need to know the property type of
an attribute. A simple static mapping between

For animations, we need to know the SVG property type for a XML attribute.

> Source/WebCore/ChangeLog:10
> +	   x can be a SVGNumberList, a SVGNumber or SVGLength. So we have to
ask every target element, if it supports

or a SVGLength.

> Source/WebCore/ChangeLog:11
> +	   the animated attribute and of which type it is. Just for CSS
properties we can use biunique static mappings between

s/beiunique static/bijective/?

> Source/WebCore/ChangeLog:12
> +	   the name and the type. This is done in a static map in
SVGStyledElement. All other mappings are stored in a local

s/in a/with a/

> Source/WebCore/svg/SVGAElement.cpp:112
> +    HashMap<QualifiedName, AnimatedAttributeType>& animatablePropertyMap =
this->animatablePropertyMap();

This needs a typedef, so we can use
AttributeToPropertyTypeMap& map = this->animatablePropertyMap();
And we should always use
ASSERT(map.isEmpty())
to document the pre-condition of this function call.

> Source/WebCore/svg/SVGElement.h:138
> +    HashMap<QualifiedName, AnimatedAttributeType> m_animatedAttributeMap;

I think this should go into SVGElementRareData, to save the overhead for static
non-animated files.

> Source/WebCore/svg/SVGFEMergeNodeElement.h:40
> +    virtual void ensureAnimatablePropertyMap();

Maybe we should rather name it "fillAttributeToPropertyTypeMap()" ? ensure
sounds so ... flakey.

> Source/WebCore/svg/SVGStyledLocatableElement.cpp:65
> +void SVGStyledLocatableElement::ensureAnimatablePropertyMap()
> +{
> +    SVGStyledElement::ensureAnimatablePropertyMap();
> +}

Useless.