Bug 53442 - SVGAnimateElement needs information about the animated attribute type
: SVGAnimateElement needs information about the animated attribute type
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 41761
  Show dependency treegraph
 
Reported: 2011-01-31 11:58 PST by
Modified: 2011-02-10 17:17 PST (History)


Attachments
Patch (110.64 KB, patch)
2011-01-31 12:44 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (99.77 KB, patch)
2011-02-01 03:32 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (99.64 KB, patch)
2011-02-01 06:39 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (100.23 KB, patch)
2011-02-01 09:11 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (116.25 KB, patch)
2011-02-08 14:26 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (116.29 KB, patch)
2011-02-09 10:16 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (127.04 KB, patch)
2011-02-10 06:52 PST, Dirk Schulze
zimmermann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-31 11:58:14 PST
SVGAnimateElement needs information about the animated attribute type
------- Comment #1 From 2011-01-31 12:44:32 PST -------
Created an attachment (id=80672) [details]
Patch
------- Comment #2 From 2011-02-01 03:32:55 PST -------
Created an attachment (id=80735) [details]
Patch
------- Comment #3 From 2011-02-01 04:14:16 PST -------
(From update of attachment 80735 [details])
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.
------- Comment #4 From 2011-02-01 06:39:05 PST -------
Created an attachment (id=80757) [details]
Patch
------- Comment #5 From 2011-02-01 07:29:53 PST -------
(From update of attachment 80757 [details])
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.
------- Comment #6 From 2011-02-01 09:11:28 PST -------
Created an attachment (id=80765) [details]
Patch
------- Comment #7 From 2011-02-03 10:43:30 PST -------
(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...

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?
------- Comment #8 From 2011-02-03 10:49:12 PST -------
(In reply to comment #7)
> (From update of attachment 80765 [details] [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 :-(
------- Comment #9 From 2011-02-08 14:26:18 PST -------
Created an attachment (id=81693) [details]
Patch
------- Comment #10 From 2011-02-09 09:29:32 PST -------
Upload a new patch in a few moments.
------- Comment #11 From 2011-02-09 10:16:00 PST -------
Created an attachment (id=81828) [details]
Patch
------- Comment #12 From 2011-02-10 06:52:59 PST -------
Created an attachment (id=81974) [details]
Patch
------- Comment #13 From 2011-02-10 09:05:22 PST -------
(From update of attachment 81974 [details])
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)
------- Comment #14 From 2011-02-10 11:17:47 PST -------
Committed r78249: <http://trac.webkit.org/changeset/78249>
------- Comment #15 From 2011-02-10 14:51:15 PST -------
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 :-/)
------- Comment #16 From 2011-02-10 17:17:05 PST -------
I filed bug 54259 for the warning.

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