Bug 10750 – SVG needs SVGAnimatedTypeNamePropertyName<ClassName> classes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVG needs SVGAnimatedTypeNamePropertyName<ClassName> classes

WildFox and Maciej and I have talked about this before, I'm just now putting it down in the bug system for wider viewing:

In order to finally make the various SVGAnimated* types work again from the javascript bindings we will need to create a set of new classes to function as "tear-offs" from DOM objects, and to allow for proper integration with the exiting auto-bindings system.  The system of classes we have designed for these SVGAnimated* tear-off is as follows:

1.  Abstract base-classes for each of various SVGAnimated types.  (SVGAnimatedBoolean, SVGAnimatedString, etc).  These could be created based from a SVGAnimatedTemplate<T> similar to what we had with the old animation system.
2.  Per-property, per-class SVGAnimatedType subclass for each of the SVGAnimated* properties which we support.  A macro can be used to generate each of these.  They should be in the form SVGAnimatedTypeNamePropertyName<ElementClassName>.
3.  Extend the existing ANIMATED_PROPERTY macros to generate animatedPropertyName() functions to create and return the SVGAnimated* tear-off classes.
4.  Then we can just turn on all of the SVGAnimated*.idl files and viola! bindings should "just work"

These various SVGAnimatedTypeNamePropertyName classes will look something like this:

template<TypeName>
class SVGAnimatedTemplate<TypeName> : public RefPtr<SVGAnimatedTemplate<TypeName> > {
public:
    virtual TypeName baseVal() = 0;
    virtual setBaseVal(TypeName) = 0;
    virtual TypeName animVal() = 0;
    virtual setAnimVal(TypeName) = 0;
protected:
    RefPtr<SVGElement> m_element;
};

template <ClassName>
class SVGAnimatedTypeNamePropertyName<ClassName> : public SVGAnimatedTemplate<TypeName>  {
public:
    SVGAnimatedTypeNamePropertyName(SVGElement*);
    virtual ~ SVGAnimatedTypeNamePropertyName();
    virtual TypeName baseVal()
    {
            return (static_cast<ClassName*>(m_element)->propertyNameBaseValue();
    }
    virtual void setBaseVal(TypeName newBaseVal, ExceptionCode&) // EC may be optional, depending
    {
           static_cast<ClassName*>(m_element)->setPropertyNameBaseVal(newBaseVal);
    }
    virtual TypeName animVal()
    {
            return static_cast<ClassName*>(m_element)->propertyName();
    }
    virtual void setAnimVal(TypeName newAnimVal, ExceptionCode&) // EC may be optional, depending
    {
           static_cast<ClassName*>(m_element)->setPropertyName(newAnimVal);
    }
};

That should be about it.  This shouldn't take WildFox (or anyone else familiar with the JS system and templates) more than a few hours of work.  Turning on all of the SVGAnimated*.idl files can always be done at a later time.  However once this is complete, turning them all on should be possible.

We can either use a single manually-generated SVGAnimatedTypes.h file (which uses some Macros to write all these classes), or someone could write a little perl script to crawl the IDLs and generate these for us.  I'm not sure which would be cleaner.

[Nikolas Zimmermann]

Created attachment 10462 [details]
Changes in SVGAnimatedTemplate

[Nikolas Zimmermann]

Created attachment 10463 [details]
Changes in the CodeGenerator's

[Nikolas Zimmermann]

Created attachment 10464 [details]
Remove all SVGAnimated*.cpp / .h files

This patch just removes files.

[Nikolas Zimmermann]

Created attachment 10465 [details]
Changes in svg/SVG*.h files (new param in a macro)

[Nikolas Zimmermann]

Ok it doesn't build on OSX, the same perl code which works nice on linux doesn't work on OSX. Hmpf, need to look into it tomorrow - please don't
land yet.

Will already incorporate some suggestions from Eric.
The new patch (coming tomorrow) will also be done in one single patch,
as Darin suggested (I'm convinced it makes more sense :-).

Night, Niko

[Nikolas Zimmermann]

Created attachment 10494 [details]
Updated complete patch

Incorporated changes when discussing this with Eric.
JS generation is much nicer handled now, SVGLists work again etc. etc.
(See ChangeLog :-)

This includes OSX & Qt platform build changes, works on both platforms.
No new regressions introduced, none fixed (will fix some tests, if the
element's idl files are actually generated - this is another patch)

As Darin suggested I've put all in a single patch. Now it's quite large
I hope someone can find some time to review until Tuesday, which is
my last hacking day for the next week(s) :/

Thanks in advance to the brave souls crawling through the patch...
Niko

[Eric Seidel (no email)]

Comment on attachment 10494 [details]
Updated complete patch

This is going to take a couple passes.  314k is a big patch to try and review in one go.

darin is never a fan of these comments (with ellipses):
+    # Start actual generation...

This should probably be a subroutine:
-        $implIncludes{"${type}.h"} = 1;
+        if ($type ne "SVGRect" and
+            $type ne "SVGPoint" and
+            $type ne "SVGNumber") {
+            $implIncludes{"${type}.h"} = 1;
+        }

This also seems rather specific and probably should be a sub-routine:
-    push(@headerContent, "class $implClassName;\n\n");
-    
+    push(@headerContent, "class $implClassName;\n\n") unless($codeGenerator->IsSVGAnimatedType($implClassName));
+

Again, subroutine:

+  if ($className =~ /^JSSVGAnimated/) {
+    my $type = $interfaceName;
+    $type =~ s/SVGAnimated//; 
+
+    if ($type eq "SVGRect" or
+        $type eq "SVGPoint" or
+        $type eq "SVGNumber") {
+      $implIncludes{"JSSVG$type.h"} = 1;
+    } else {
+      push(@implContentHeader, "#include \"PlatformString.h\"\n") if($type eq "String");
+    }
+  }

+    return 1 if $type eq "SVGAnimatedAngle" or
+                $type eq "SVGAnimatedBoolean" or
+                $type eq "SVGAnimatedEnumeration" or
+                $type eq "SVGAnimatedInteger" or


This would be easier written by using:
$type =~ s/SVGAnimated/
%w{ Angle Boolean Enumeration }.contains $type, or something similar.
(I think that works in perl, I know it does in ruby...)

Tabs:
+	JSSVGAnimatedAngle.h \
+	JSSVGAnimatedBoolean.h \


I don't understand what "nodptr" is.  Seems like a bad name if nothing else:
+  interface [nodptr, Conditional=SVG] SVGAnimatedPoints { 

darin prefers new Foo; in these sorts of cases (no ()):
+    , m_tableValues(new SVGNumberList())
Yet another thing which should be added to the style guidelines.

I've been removing KSVG_ as well from these:
-#endif // KSVG_SVGPreserveAspectRatioImpl_H
+#endif // KSVG_SVGPreserveAspectRatio_H

since it's now sorta all WebCore (not that we don't like KSVG!)

No need for the argument name here:
+        SVGTransform* createSVGTransformFromMatrix(SVGMatrix* matrix) const;

What a fabulous change:
-    viewBoxBaseValue()->setX(x);
-    viewBoxBaseValue()->setY(y);
-    viewBoxBaseValue()->setWidth(w);
-    viewBoxBaseValue()->setHeight(h);
+    setViewBoxBaseValue(FloatRect(x, y, w, h));

Normally we do ! instead of == 0, also if( needs a space:
+    if(viewBoxRect.width() == 0 || viewBoxRect.height() == 0)

Conditional=SVG seems redundant if everything is inside module SVG:
+module svg
+{
+  interface [Conditional=SVG] SVGAnimatedRect { 


No need to explicitly initialize m_vector:

+        SVGListBase() : m_vector() { }


These are beautiful:
+	// Specialization for String...
+	template<>
+    class SVGList<String> : public SVGListBase<String>

This needs to move off of DeprecatedString, but not necessarily in this patch:

+void SVGStringList::reset(const DeprecatedString& str)
 {

We need to have a good method of codifying this:
+             attribute core::DOMString baseVal;
+                         // raises DOMException on setting

I think you already have a FIXME in SVGAnimatedTemplate.  WE shoudl file a bug as well though.


again, confused by "nodptr"

+  interface [nodptr, Conditional=SVG] SVGAnimatedPathData { 

Seems silly to have a separate line for return here:
+    RefPtr<ClassName::SVGAnimatedTemplate##UpperProperty> ret; \
+    ret = new ClassName::SVGAnimatedTemplate##UpperProperty(context); \
+    return ret; \

here again:
+    RefPtr<ClassName::SVGAnimatedTemplate##UpperProperty> ret; \
+    ret = new ClassName::SVGAnimatedTemplate##UpperProperty(this); \
+    return ret; \

No need to create a new floatRect here on property at a time, or?
+            static_cast<RenderSVGContainer*>(renderer())->setViewBox(FloatRect(viewBox().x(), viewBox().y(), viewBox().width(), viewBox().height()));

and again:
+    rootContainer->setViewBox(FloatRect(viewBox().x(), viewBox().y(), viewBox().width(), viewBox().height()));

I'm not sure why this is necessary:
+// Special handling for WebCore::FloatRect
+template<>
+inline FloatRect SVGDocumentExtensions::baseValue<FloatRect>(const SVGElement* element, const AtomicString& propertyName) const

You can have pointer and non-pointer specializations.  Vector uses something like that.

This may be simply the most beautiful patch I have ever seen. :)

I would r+ it right here and now, but since you don't (yet!) have commit privileges, I think we should go one more round on this one to fix a few of the little issues I've mentioned above.

I *can't wait* to see this land!!

[Eric Seidel (no email)]

Actually, my comments above were really really tiny issues.  This could be landed as-is.  Probably best still to give it another once-over before it's put to rest though.

[Nikolas Zimmermann]

Heya Eric,

thanks for the quick review.

> darin is never a fan of these comments (with ellipses):
> +    # Start actual generation...

Fixed all places where "..." was appended.
 
> This should probably be a subroutine:
> -        $implIncludes{"${type}.h"} = 1;
> +        if ($type ne "SVGRect" and
> +            $type ne "SVGPoint" and
> +            $type ne "SVGNumber") {
> +            $implIncludes{"${type}.h"} = 1;
> +        }

Fixed, now lives in "AvoidInclusionOfType" method.

> This also seems rather specific and probably should be a sub-routine:
> -    push(@headerContent, "class $implClassName;\n\n");
> -    
> +    push(@headerContent, "class $implClassName;\n\n")
> unless($codeGenerator->IsSVGAnimatedType($implClassName));
> +

Fixed, now lives in "AddClassForwardIfNeeded" method.
 
> +  if ($className =~ /^JSSVGAnimated/) {
> +    my $type = $interfaceName;
> +    $type =~ s/SVGAnimated//; 
> +
> +    if ($type eq "SVGRect" or
> +        $type eq "SVGPoint" or
> +        $type eq "SVGNumber") {
> +      $implIncludes{"JSSVG$type.h"} = 1;
> +    } else {
> +      push(@implContentHeader, "#include \"PlatformString.h\"\n") if($type eq
> "String");
> +    }
> +  }

Fixed, now lives in "AddIncludesForSVGAnimatedType" method.

> 
> +    return 1 if $type eq "SVGAnimatedAngle" or
> +                $type eq "SVGAnimatedBoolean" or
> +                $type eq "SVGAnimatedEnumeration" or
> +                $type eq "SVGAnimatedInteger" or

Fixed using hashes, also converted IsPrimitiveType to use a hash.

> Tabs:
> +       JSSVGAnimatedAngle.h \
> +       JSSVGAnimatedBoolean.h \

Fixed.

> I don't understand what "nodptr" is.  Seems like a bad name if nothing else:
> +  interface [nodptr, Conditional=SVG] SVGAnimatedPoints { 

Heh, that must have been a leftover in those IDLs from KDOM days, fixed.

 > darin prefers new Foo; in these sorts of cases (no ()):
> +    , m_tableValues(new SVGNumberList())
> Yet another thing which should be added to the style guidelines.

Fixed all occourences of that.

> I've been removing KSVG_ as well from these:
> -#endif // KSVG_SVGPreserveAspectRatioImpl_H
> +#endif // KSVG_SVGPreserveAspectRatio_H

Ah ok, not doing that in this patch though.

> No need for the argument name here:
> +        SVGTransform* createSVGTransformFromMatrix(SVGMatrix* matrix) const;

Fixed.
 
> Normally we do ! instead of == 0, also if( needs a space:
> +    if(viewBoxRect.width() == 0 || viewBoxRect.height() == 0)
> 
Fixed.

> Conditional=SVG seems redundant if everything is inside module SVG:
> +module svg
> +{
> +  interface [Conditional=SVG] SVGAnimatedRect { 

Hm, I wondered about that too, though not touching it now.
 
> No need to explicitly initialize m_vector:
> +        SVGListBase() : m_vector() { }

Fixed.

> This needs to move off of DeprecatedString, but not necessarily in this patch:
> 
> +void SVGStringList::reset(const DeprecatedString& str)

Sure, DeprecatedString is useless there. String will do.

> We need to have a good method of codifying this:
> +             attribute core::DOMString baseVal;
> +                         // raises DOMException on setting
> 
> I think you already have a FIXME in SVGAnimatedTemplate.  WE shoudl file a bug
> as well though.
Oh yes, bug fileing is your area, eh? :-)

> Seems silly to have a separate line for return here:
> +    RefPtr<ClassName::SVGAnimatedTemplate##UpperProperty> ret; \
> +    ret = new ClassName::SVGAnimatedTemplate##UpperProperty(context); \
> +    return ret; \
> 
> here again:
> +    RefPtr<ClassName::SVGAnimatedTemplate##UpperProperty> ret; \
> +    ret = new ClassName::SVGAnimatedTemplate##UpperProperty(this); \
> +    return ret; \

Fixed, to directly return the RefPtr.

> No need to create a new floatRect here on property at a time, or?
> +           
> static_cast<RenderSVGContainer*>(renderer())->setViewBox(FloatRect(viewBox().x(),
> viewBox().y(), viewBox().width(), viewBox().height()));
> 
> and again:
> +    rootContainer->setViewBox(FloatRect(viewBox().x(), viewBox().y(),
> viewBox().width(), viewBox().height()));

Hehe, that's a leftover, good catch - fixed to just use viewBox().

> I'm not sure why this is necessary:
> +// Special handling for WebCore::FloatRect
> +template<>
> +inline FloatRect SVGDocumentExtensions::baseValue<FloatRect>(const SVGElement*
> element, const AtomicString& propertyName) const
> 
> You can have pointer and non-pointer specializations.  Vector uses something
> like that.

Hm, where does it use that? Maybe anyone can give some hints?
I needed these specializations - left them in for now.

> This may be simply the most beautiful patch I have ever seen. :)
/me blushes. Thanks a lot.
 
> I *can't wait* to see this land!!
Me too! I'll test the patch on OSX now, if it works, I'll upload it.
Expect the patch to be here in ~ 30 minutes :-)

[Nikolas Zimmermann]

Created attachment 10503 [details]
Final patch

Incorporated Eric's suggestions, all tests pass, no new regressions, build tested on Linux & OSX.

[Eric Seidel (no email)]

Comment on attachment 10503 [details]
Final patch

r=me