WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 375744
[details]
Patch
Alex Christensen
Comment 7
2019-08-07 13:37:14 PDT
Created
attachment 375745
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug