Bug 20051 - SVGAnimated* properties macro magic needs to be rewritten
Summary: SVGAnimated* properties macro magic needs to be rewritten
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
Depends on: 10745 20052
Blocks: 12171
  Show dependency treegraph
Reported: 2008-07-16 05:15 PDT by Nikolas Zimmermann
Modified: 2019-08-07 13:48 PDT (History)
6 users (show)

See Also:

Initial patch (208.56 KB, patch)
2008-07-16 17:48 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (211.84 KB, patch)
2008-07-17 11:22 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (1.70 KB, patch)
2019-08-07 13:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (1.70 KB, patch)
2019-08-07 13:37 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2008-07-16 05:15:30 PDT
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

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.
Comment 1 Nikolas Zimmermann 2008-07-16 17:48:53 PDT
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.
Comment 2 Nikolas Zimmermann 2008-07-17 11:22:50 PDT
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)
Comment 3 Oliver Hunt 2008-07-18 19:10:02 PDT
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
Comment 4 Nikolas Zimmermann 2008-07-19 09:05:35 PDT
Landed in r35248.
Comment 5 Alex Christensen 2019-08-07 13:23:00 PDT
Reopening to attach new patch.
Comment 6 Alex Christensen 2019-08-07 13:23:01 PDT
Created attachment 375744 [details]
Comment 7 Alex Christensen 2019-08-07 13:37:14 PDT
Created attachment 375745 [details]
Comment 8 Said Abou-Hallawa 2019-08-07 13:48:16 PDT
(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.