RESOLVED FIXED 20051
SVGAnimated* properties macro magic needs to be rewritten
https://bugs.webkit.org/show_bug.cgi?id=20051
Summary SVGAnimated* properties macro magic needs to be rewritten
Nikolas Zimmermann
Reported 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 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.
Attachments
Initial patch (208.56 KB, patch)
2008-07-16 17:48 PDT, Nikolas Zimmermann
no flags
Updated patch (211.84 KB, patch)
2008-07-17 11:22 PDT, Nikolas Zimmermann
no flags
Patch (1.70 KB, patch)
2019-08-07 13:23 PDT, Alex Christensen
no flags
Patch (1.70 KB, patch)
2019-08-07 13:37 PDT, Alex Christensen
no flags
Nikolas Zimmermann
Comment 1 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.
Nikolas Zimmermann
Comment 2 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)
Oliver Hunt
Comment 3 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
Nikolas Zimmermann
Comment 4 2008-07-19 09:05:35 PDT
Landed in r35248.
Alex Christensen
Comment 5 2019-08-07 13:23:00 PDT
Reopening to attach new patch.
Alex Christensen
Comment 6 2019-08-07 13:23:01 PDT
Alex Christensen
Comment 7 2019-08-07 13:37:14 PDT
Said Abou-Hallawa
Comment 8 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.
Note You need to log in before you can comment on or make changes to this bug.