[Webkit-unassigned] [Bug 20051] New: SVGAnimated* properties macro magic needs to be rewritten

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20051

           Summary: SVGAnimated* properties macro magic needs to be
                    rewritten
           Product: WebKit
           Version: 526+ (Nightly build)
          Platform: Macintosh
        OS/Version: Mac OS X 10.5
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: SVG
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: zimmermann at kde.org
                CC: eric at webkit.org, oliver at apple.com, koivisto at iki.fi


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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.