SVGAnimated* properties macro magic needs to be rewritten

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

SVGAnimated* properties concept, and the associated macros have several problems:

- the code is not debuggable, or at least it's very hard (manual macro expansion)
- the code is not readable, it hides all the complexity, for handling refcounted & POD animatable types, where templates & traits would fit much better
- the code uses WAY to much virutal function calls

Proposal:
Introduce a new templatified class, SVGAnimatedProperty, with four parameters:
- typename OwnerType (the class which contains us, ie. SVGCircleElement)
The SVGAnimatedProperty class should contain helper templates, that are able to figure out the right "OwnerElement" for a certain "OwnerType". For SVGElement-derived classes they are always equal, ie. for SVGCircleElement OwnerType == OwnerElement, though for ie. SVGURIReference, the OwnerElement is a SVGElement object (retrieved using the virtual contextElement() function).

- typename AnimatedType (the animatable type, ie. SVGLength or SVGTransformList)
The SVGAnimatedProperty class uses helper templates from SVGAnimatedTemplate to figure out the "StorableType" and "DecoratedType" belonging to a certain "AnimatedType' ie. for SVGLength, all three types are equal. For ie. SVGTransformList, the StorableType is RefPtr<SVGTransformList> and the DecoratedType is SVGTransformList*.

- const char* TagName / - const char* PropertyName
Yes, two const char* template parameter to differentiate between the tag/property combinations. 
Previously we used different classnames to achive this goal. ie SVGCircleElement contained a sub-class SVGAnimatedTemplateX (for the "x" property). 

The dom/make_names.pl script can easily be modified to expose stuff like const char* SVGNames::circleTagString = "circle";.

So basically what the old ANIMATED_PROPERTY_DECLARATIONS macro did, has been moved into a templatified class. Though the macro _STILL_ remains for the ease of declarating these properties, though it does NOT contain any logic anymore, but forward the calls to the SVGAnimatedProperty<...> classes, example:

    // Helper macro used to register animated properties within SVG* classes
    #define ANIMATED_PROPERTY_DECLARATIONS(OwnerType, ElementTag, AttributeTag, AnimatedType, UpperProperty, LowerProperty) \
    private: \
        typedef SVGAnimatedProperty<OwnerType, AnimatedType, ElementTag, AttributeTag> SVGAnimatedProperty##UpperProperty; \
        typedef SVGAnimatedTypeValue<AnimatedType>::DecoratedType DecoratedTypeFor##UpperProperty; \
        SVGAnimatedProperty##UpperProperty m_##LowerProperty; \
    public: \
        DecoratedTypeFor##UpperProperty LowerProperty() const { return m_##LowerProperty.value(); } \
        void set##UpperProperty(DecoratedTypeFor##UpperProperty type) { m_##LowerProperty.setValue(type); } \
        DecoratedTypeFor##UpperProperty LowerProperty##BaseValue() const { return m_##LowerProperty.baseValue(); } \
        void set##UpperProperty##BaseValue(DecoratedTypeFor##UpperProperty type) { m_##LowerProperty.setBaseValue(type); } \
        PassRefPtr<SVGAnimatedProperty##UpperProperty::TearOff> LowerProperty##Animated() const { return m_##LowerProperty.animatedTearOff(); } \
        void synchronize##UpperProperty() const { m_##LowerProperty.synchronize(); }

This allows us to easily declare these properties, while preserving debugability.
The ANIMATED_PROPERTY_DEFINITIONS macro has vanished, it's not needed anymore, as we initialize the properties in the class constructor now, for example for SVGCircleElement:

-    , m_cx(this, LengthModeWidth)
+    , m_cx(this, SVGNames::cxAttr, LengthModeWidth)

This even allows us to specify the default values as 4th parameter. (The SVGAnimatedProperty constructor forwards these arguments to the constructor of the animated type, ie. SVGLength in the case above).

NOTE: All methods of SVGAnimatedProperty are inlined, non-virtual methods, the ANIMATED_PROPERTY_EMPTY_DECLARATIONS and ANIMATED_PROPERTY_FORWARD_DECLARATIONS macros are COMPLETLY gone, no need to define ie. SVGURIReferences' href property in SVGElement anymore (which was awkward anyway).

The whole SVG<->XML synchronization part does NOT need any virtual function call anymore (on the SVG side), invokeSVGPropertySynchronization & friends aren't virtual anymore, and instead live directly in SVGElement.

This finally allows me to kill the SVGLength's "const SVGElement* context" parameter, saving a lot of memory - the new concept easily allows to remove this hack.

Next benefit: Removing the virtual contextElement() function from all SVGElement derived classes, there is a central contextElement() function in SVGElement now, and some pure-virtual contextElement() function in classes like SVGURIReference etc.

As you can see this approach has dozens of benefits. Need to split up code in individual patches, this is just the master bug.

[Nikolas Zimmermann]

Created attachment 22325 [details]
Initial patch

Forgot to mention in the bug description, that SVG<->XML synchronization shouldn't need any callbacks. This patch also fixes that.

[Nikolas Zimmermann]

Created attachment 22343 [details]
Updated patch

Slightly changed patch, let updateAnimated* take const String& instead of StringImpl, refactor synchronization code in helper function (used by SVGPolyElement in a follow-up patch)

[Oliver Hunt]

Comment on attachment 22343 [details]
Updated patch

Okay, afaict this looks sane, but i'd like antti to look at it at some point.  r=me

[Nikolas Zimmermann]

Landed in r35248.

[Alex Christensen]

Reopening to attach new patch.

[Alex Christensen]

Created attachment 375744 [details]
Patch

[Alex Christensen]

Created attachment 375745 [details]
Patch

[Said Abou-Hallawa]

(In reply to Alex Christensen from comment #7)
> Created attachment 375745 [details]
> Patch

I think this patch should be uploaded to https://bugs.webkit.org/show_bug.cgi?id=200514.