Bug 112382 – [SVG] Avoid static casts in DECLARE_ANIMATED_PROPERTY macros

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: SVG

[Florin Malita]

Currently, DECLARE_ANIMATED_PROPERTY expands a couple of static casting methods (SVGAnimatedPropertyMacros.h):

148	    static PassRefPtr<SVGAnimatedProperty> lookupOrCreate##UpperProperty##Wrapper(SVGElement* maskedOwnerType) \
149	    { \
150	        ASSERT(maskedOwnerType); \
151	        UseOwnerType* ownerType = static_cast<UseOwnerType*>(maskedOwnerType); \
152	        return SVGAnimatedProperty::lookupOrCreateWrapper<UseOwnerType, TearOffType, PropertyType>(ownerType, LowerProperty##PropertyInfo(), ownerType->m_##LowerProperty.value); \
153	    } \
154	\
155	    static void synchronize##UpperProperty(SVGElement* maskedOwnerType) \
156	    { \
157	        ASSERT(maskedOwnerType); \
158	        UseOwnerType* ownerType = static_cast<UseOwnerType*>(maskedOwnerType); \
159	        ownerType->synchronize##UpperProperty(); \
160	    } \


Now that the maskedOwnerType parameter is no longer an opaque void*, we should investigate converting these static casts to toSVG* calls (which can perform some ASSERT_WITH_SECURITY_IMPLICATIONS-time type checking).

Even though the macros provide some welcome encapsulation, this is not exactly straightforward for a couple of reason:

1) in order to expand the correct toSVG*() call, the macro needs access to the type name; but the type name is not passed to DECLARE_ANIMATED_PROPERTY, only to the BEGIN_DECLARE_ANIMATED_PROPERTIES (which typedef's OwnerType -> UseOwnerType). Since UseOwnerType is a typedef and not a macro parameter for DECLARE_ANIMATED_PROPERTY, we cannot immediately stringify it to construct the toSVG* call.

2) many SVG elements with animatable attributes are lacking the needed toSVGXYZ() converting functions.

I don't think we can do much for automating #2: the functions are heterogeneous, since type checking rules vary.

For #1 we could modify DECLARE_ANIMATED_PROPERTY to also take an OwnerType parameter, just like BEGIN_DECLARE_ANIMATED_PROPERTIES. But this would touch gobs of lines and add redundant information. Alternatively, I have an idea that may work out:

* define a thin inlined type-converting wrapper in BEGIN_DECLARE_ANIMATED_PROPERTIES, which simply delegates to the right toSVGXYZ() function (this is doable here because the type name is a macro parameter); the name for this wrapper should be constant (e.g., "fromSVGElement") such that it can then be referenced from DECLARE_ANIMATED_PROPERTY without any macro tricks.
* replace the static casts in DECLARE_ANIMATED_PROPERTY with straight calls to the constant named wrapper

Putting these together yields a change on these lines for SVGAnimatedPropertyMacros.h:

 #define BEGIN_DECLARE_ANIMATED_PROPERTIES(OwnerType) \
 public: \
     static SVGAttributeToPropertyMap& attributeToPropertyMap(); \
     virtual SVGAttributeToPropertyMap& localAttributeToPropertyMap() \
     { \
         return attributeToPropertyMap(); \
     } \
-    typedef OwnerType UseOwnerType;
+    typedef OwnerType UseOwnerType; \
+\
+private: \
+    static OwnerType* fromSVGElement(SVGElement* element) \
+    { \
+        return to##OwnerType(element); \
+    }
 
 #define DECLARE_ANIMATED_PROPERTY(TearOffType, PropertyType, UpperProperty, LowerProperty) \
 public: \
@@ -148,14 +154,14 @@ private: \
     static PassRefPtr<SVGAnimatedProperty> lookupOrCreate##UpperProperty##Wrapper(SVGElement* maskedOwnerType) \
     { \
         ASSERT(maskedOwnerType); \
-        UseOwnerType* ownerType = static_cast<UseOwnerType*>(maskedOwnerType); \
+        UseOwnerType* ownerType = UseOwnerType::fromSVGElement(maskedOwnerType); \
         return SVGAnimatedProperty::lookupOrCreateWrapper<UseOwnerType, TearOffType, PropertyType>(ownerType, LowerProp
     } \
 \
     static void synchronize##UpperProperty(SVGElement* maskedOwnerType) \
     { \
         ASSERT(maskedOwnerType); \
-        UseOwnerType* ownerType = static_cast<UseOwnerType*>(maskedOwnerType); \
+        UseOwnerType* ownerType = UseOwnerType::fromSVGElement(maskedOwnerType); \
         ownerType->synchronize##UpperProperty(); \
     } \


Then for each animatable SVG element header:

* add a toSVGXYZ() function if missing (most cases)
* forward declare the above (the animated property macros are used before the type converting function is defined)

Does the above sounds like an acceptable approach?


P.S: An argument could also be made for refactoring the existing to*() functions into static methods of their respective type. That would avoid the need for unique naming (as they no longer live in the same WebCore namespace), and a common name would simplify the task above. For example:

toSVGElement(e)                -> SVGElement::fromElement(e)
toSVGSVGElement(e)             -> SVGSVGElement::fromElement(e)
toSVGStyledLocatableElement(e) -> SVGStyleLocatableElement::fromElement(e)
...

Personally, I would prefer this form - but it's a large change for relatively little gain.

[Brent Fulgham]

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.