[Webkit-unassigned] [Bug 112382] New: [SVG] Avoid static casts in DECLARE_ANIMATED_PROPERTY macros

[bugzilla-daemon at webkit.org]

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

           Summary: [SVG] Avoid static casts in DECLARE_ANIMATED_PROPERTY
                    macros
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: Unspecified
        OS/Version: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: SVG
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: fmalita at chromium.org
                CC: zimmermann at kde.org, krit at webkit.org,
                    inferno at chromium.org, pdr at google.com,
                    schenney at chromium.org


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.

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