Bug 53442

Summary: SVGAnimateElement needs information about the animated attribute type
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: thakis, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41761    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch zimmermann: review+

Description Dirk Schulze 2011-01-31 11:58:14 PST
SVGAnimateElement needs information about the animated attribute type
Comment 1 Dirk Schulze 2011-01-31 12:44:32 PST
Created attachment 80672 [details]
Patch
Comment 2 Dirk Schulze 2011-02-01 03:32:55 PST
Created attachment 80735 [details]
Patch
Comment 3 Nikolas Zimmermann 2011-02-01 04:14:16 PST
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.
Comment 4 Dirk Schulze 2011-02-01 06:39:05 PST
Created attachment 80757 [details]
Patch
Comment 5 Nikolas Zimmermann 2011-02-01 07:29:53 PST
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.
Comment 6 Dirk Schulze 2011-02-01 09:11:28 PST
Created attachment 80765 [details]
Patch
Comment 7 Nikolas Zimmermann 2011-02-03 10:43:30 PST
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?
Comment 8 Dirk Schulze 2011-02-03 10:49:12 PST
(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 :-(
Comment 9 Dirk Schulze 2011-02-08 14:26:18 PST
Created attachment 81693 [details]
Patch
Comment 10 Dirk Schulze 2011-02-09 09:29:32 PST
Upload a new patch in a few moments.
Comment 11 Dirk Schulze 2011-02-09 10:16:00 PST
Created attachment 81828 [details]
Patch
Comment 12 Dirk Schulze 2011-02-10 06:52:59 PST
Created attachment 81974 [details]
Patch
Comment 13 Nikolas Zimmermann 2011-02-10 09:05:22 PST
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)
Comment 14 Dirk Schulze 2011-02-10 11:17:47 PST
Committed r78249: <http://trac.webkit.org/changeset/78249>
Comment 15 Nico Weber 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 Nico Weber 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.)