Bug 53442 – SVGAnimateElement needs information about the animated attribute type

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVGAnimateElement needs information about the animated attribute type

[Dirk Schulze]

Created attachment 80672 [details]
Patch

[Dirk Schulze]

Created attachment 80735 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 80735 [details]
Patch

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.

[Dirk Schulze]

Created attachment 80757 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 80757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80757&action=review

Almost there, still r- though.

> Source/WebCore/svg/SVGAnimateElement.cpp:91
> +    AnimatedAttributeType type = targetElement()->animatedPropertyTypeForAttribute(QualifiedName(nullAtom, attribute, nullAtom));
>      // FIXME: We need a full property table for figuring this out reliably.
> -    if (hasTagName(SVGNames::animateColorTag))
> +    if (hasTagName(SVGNames::animateColorTag) || type == AnimatedColor)
>          return ColorProperty;

You don't need to ask the targetElement(), if hasTagNames(SVGNames::animateColorTag) is true. Make it a early exit shortcut.

> Source/WebCore/svg/SVGCircleElement.cpp:139
> +    AttributeToPropertyTypeMap& animatablePropertyMap = this->animatablePropertyMap();

s/animatablePropertyMap/attributeToPropertyTypeMap/ ?

> Source/WebCore/svg/SVGElement.h:140
> +    AttributeToPropertyTypeMap m_animatedAttributeMap;

Obsolete, please remove.

> Source/WebCore/svg/SVGElementRareData.h:60
> +    HashMap<QualifiedName, AnimatedAttributeType>& animatedAttributeMap() { return m_animatedAttributeMap; }

Use the typdef.

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

Ditto.

> Source/WebCore/svg/SVGGlyphElement.cpp:83
> +void SVGGlyphElement::svgAttributeChanged(const QualifiedName& attrName)
> +{
> +    if (attrName == SVGNames::dAttr)
> +        invalidateGlyphCache();
> +}

This seems unrelated.

> Source/WebCore/svg/SVGGlyphElement.h:120
> +    virtual void svgAttributeChanged(const QualifiedName&);

Ditto.

> Source/WebCore/svg/SVGStyledElement.cpp:204
> +AttributeToPropertyTypeMap& SVGStyledElement::cssPropertyToTypeMap()

Make it a static inline.

> Source/WebCore/svg/SVGStyledElement.h:86
> +    AttributeToPropertyTypeMap& cssPropertyToTypeMap();

No need for this, if you convert it to be a static inline.

[Dirk Schulze]

Created attachment 80765 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 80765 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80765&action=review

Ok, I'm stopping the review here, as I think the general concept is wrong, after thinking about it again.
We just need a map between eg. SVGRectElements property types for any attribute it supports.
When storing this information per element in eg. SVGElement or SVGElementRareData (aka. storing the HashMap), we're basically storing the same information for _each_ rect in the DOM.
Consider 100 <rect> elements, each holds such a HashMap...

We need to discuss this a bit further.

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

s/Just for/For/. s/explicite/explicit/

> Source/WebCore/ChangeLog:13
> +        HashMap in SVGElement. This map gets filled once with the function fillAttributeToPropertyTypeMap and needs to be

s/SVGElement/SVGElementRareData/

> Source/WebCore/ChangeLog:14
> +        included in every SVG element.

s/included/implemented/

> Source/WebCore/ChangeLog:16
> +        No change of functionality, so no new test cases.

Isn't it possible now to animate types, that didn't work before?

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

No need for this function, if you're not doing anything? Is the implementation missing so far?

[Dirk Schulze]

(In reply to comment #7)
> (From update of attachment 80765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80765&action=review
> 
> Ok, I'm stopping the review here, as I think the general concept is wrong, after thinking about it again.
> We just need a map between eg. SVGRectElements property types for any attribute it supports.
> When storing this information per element in eg. SVGElement or SVGElementRareData (aka. storing the HashMap), we're basically storing the same information for _each_ rect in the DOM.
> Consider 100 <rect> elements, each holds such a HashMap...
Totally agree. Unsure why I did not see it on the first patch :-(

[Dirk Schulze]

Created attachment 81693 [details]
Patch

[Dirk Schulze]

Upload a new patch in a few moments.

[Dirk Schulze]

Created attachment 81828 [details]
Patch

[Dirk Schulze]

Created attachment 81974 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 81974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81974&action=review

r=me with some fixups:

> Source/WebCore/ChangeLog:9
> +        attribute name and a type is not possible, since one attribute name can be bounded to different property types:

s/bounded/bound/

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

s/explicite mappings/explicit mapping/

> Source/WebCore/ChangeLog:13
> +        HashMaps for all SVG elements with animated properties. These maps get filled once with the function fillAttributeToPropertyTypeMap

with the function <abc> -> with the <abc> function.

> Source/WebCore/svg/SVGElement.h:92
> +    void fillAttributeToPropertyTypeMap(AttributeToPropertyTypeMap&) { }

This one is not needed, eh? Please remove it.

> Source/WebCore/svg/SVGStyledElement.cpp:273
> +    if (!cssPropertyTypeMap.contains(attrName))
> +        return AnimatedUnknown;
> +    return cssPropertyTypeMap.get(attrName);

I'd rather write:
if (cssPropertyTypeMap.contains(attrName))
    return css...get(attrName)p;
return AnimatedUnknown;

> Source/WebCore/svg/SVGStyledElement.cpp:278
> +    SVGElement::fillAttributeToPropertyTypeMap(attributeToPropertyTypeMap);

This is not needed, please remove it (even for consistency, it would be lame, to call an empty function in sVGElement here)

[Dirk Schulze]

Committed r78249: <http://trac.webkit.org/changeset/78249>

[Nico Weber]

Hi,

this patch caused this clang warning:

In file included from out/Debug/obj/gen/webkit/bindings/V8DerivedSources3.cpp:90:
In file included from out/Debug/obj/gen/webcore/bindings/V8SVGCircleElement.cpp:22:
In file included from out/Debug/obj/gen/webkit/bindings/V8SVGCircleElement.h:26:
In file included from third_party/WebKit/Source/WebCore/svg/SVGCircleElement.h:29:
In file included from third_party/WebKit/Source/WebCore/svg/SVGStyledTransformableElement.h:26:
In file included from third_party/WebKit/Source/WebCore/svg/SVGStyledLocatableElement.h:26:
third_party/WebKit/Source/WebCore/svg/SVGStyledElement.h:71:10:error: 'WebCore::SVGStyledElement::fillAttributeToPropertyTypeMap' hides overloaded virtual function [-Woverloaded-virtual]
    void fillAttributeToPropertyTypeMap(AttributeToPropertyTypeMap&);
         ^
In file included from out/Debug/obj/gen/webkit/bindings/V8DerivedSources3.cpp:90:
In file included from out/Debug/obj/gen/webcore/bindings/V8SVGCircleElement.cpp:22:
In file included from out/Debug/obj/gen/webkit/bindings/V8SVGCircleElement.h:26:
In file included from third_party/WebKit/Source/WebCore/svg/SVGCircleElement.h:25:
In file included from third_party/WebKit/Source/WebCore/svg/SVGAnimatedBoolean.h:24:
In file included from third_party/WebKit/Source/WebCore/svg/properties/SVGAnimatedStaticPropertyTearOff.h:24:
In file included from third_party/WebKit/Source/WebCore/svg/properties/SVGAnimatedProperty.h:26:
third_party/WebKit/Source/WebCore/svg/SVGElement.h:93:18: note: hidden overloaded virtual function 'WebCore::SVGElement::fillAttributeToPropertyTypeMap' declared here
    virtual void fillAttributeToPropertyTypeMap() { }
                 ^
1 error generated.


I'm not sure if SVGStyledElement::fillAttributeToPropertyTypeMap is supposed to override SVGElement::fillAttributeToPropertyTypeMap…if not, I'd recommend renaming the method in the subclass. Having an overload of a virtual method of a base class is confusing at best.

(I was just done with removing all of these warnings :-/)

[Nico Weber]

I filed bug 54259 for the warning.

(sorry, was afk when you pinged me on irc. resolving this tomorrow works for me.)