Created attachment 80672 [details] Patch

Created attachment 80735 [details] Patch

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.

Created attachment 80757 [details] Patch

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.

Created attachment 80765 [details] Patch

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?

(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 :-(

Created attachment 81693 [details] Patch

Upload a new patch in a few moments.

Created attachment 81828 [details] Patch

Created attachment 81974 [details] Patch

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)

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

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 :-/)

I filed bug 54259 for the warning. (sorry, was afk when you pinged me on irc. resolving this tomorrow works for me.)