Bug 54542 – SVG animation - analyze attribute type for animation

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

[Dirk Schulze]

Analyse the animated attribute type to specify the animation type. Get rid of PropertyType enum.

[Dirk Schulze]

Created attachment 82632 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 82632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82632&action=review

> Source/WebCore/ChangeLog:9
> +        SVG animation - analyze attribute type for animation
> +        https://bugs.webkit.org/show_bug.cgi?id=54542

Redundant.

> Source/WebCore/svg/SVGAnimateElement.cpp:136
> +    DEFINE_STATIC_LOCAL(const AtomicString, xlinkHref, ("xlink:href"));
> +
> +    // xlink:href is the only animated attribute outside of SVG.
> +    const String attributeName = this->attributeName();
> +    QualifiedName attrName(QualifiedName(nullAtom, attributeName, nullAtom));
> +    if (attributeName == xlinkHref)
> +        attrName = XLinkNames::hrefAttr;

This is definately not correct.
I'm sure we can construct a QualifiedName that works for SVG & XLink (and any other possible namespace) w/o having to declare static strings here.
Let's talk on IRC.

> Source/WebCore/svg/SVGAnimateElement.cpp:142
> +    if (type == AnimatedBoolean

A big FIXME here, that animations works this way at the moment, and we want to avoid "most types -> AnimatedString" in the future.

> Source/WebCore/svg/SVGGElement.h:54
> +    virtual void fillAttributeToPropertyTypeMap();
> +    virtual AttributeToPropertyTypeMap& attributeToPropertyTypeMap();

Why is this needed?

> Source/WebCore/svg/SVGUseElement.cpp:245
> +    attributeToPropertyTypeMap.set(XLinkNames::hrefAttr, AnimatedString);

Oops!

[Dirk Schulze]

Created attachment 82684 [details]
Patch

[Dirk Schulze]

(In reply to comment #2)
> (From update of attachment 82632 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82632&action=review

> > Source/WebCore/svg/SVGGElement.h:54
> > +    virtual void fillAttributeToPropertyTypeMap();
> > +    virtual AttributeToPropertyTypeMap& attributeToPropertyTypeMap();
> 
> Why is this needed?
Without these functions, the element takes the attribute map of SVGElement. This is equal with having no animatable attribute for this certain element at all.

[Dirk Schulze]

Created attachment 82691 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 82691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82691&action=review

Almost there, I just saw that the QName construction is not encapsulated in a good way, so we might want to fix that first. r- for now.

> Source/WebCore/svg/SVGAnimateElement.cpp:155
> +    String attributeName = this->attributeName();
> +    AnimatedAttributeType type = targetElement->animatedPropertyTypeForAttribute(QualifiedName(nullAtom, attributeName, nullAtom));
> +    if (type == AnimatedUnknown) {
> +        if (!attributeName.contains(':'))
> +            return AnimatedUnknown;
> +        // Attribute is not in the SVG namespace, check for other namespaces.
> +        QualifiedName qname = qualifiedNameForAttributeWithPrefix(targetElement, attributeName);
> +        if (qname == anyQName())
> +            return AnimatedUnknown;

Okay, I'm now questioning why this code isn't in animatedPropertyTypeForAttribute itself. How about passing a const String& to animatedPropertyTypeForAttribute, instead of a QualifiedName, and let it construct the QName on it's own.
That would remove the need to construct qnames in all of SVGAnimate* code.

There is more than one place, where you end up doing QName(nulAtom, localName, nullAtom) or QName(nullAtom, localName, nsURI) now, once here, and once in SVGAnimateTransformElement::determineAnimatedAttributeType.
Sorry, It was a bit late yesterday for me, didn't see it right from the beginning.

> Source/WebCore/svg/SVGAnimateElement.cpp:157
> +        type = targetElement->animatedPropertyTypeForAttribute(qname);
> +    }

I'd insert another shortcut here.
if (type == AnimatedUnknown)
    return type;

[Dirk Schulze]

Created attachment 82848 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 82848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82848&action=review

Great job, dirk, almost there, I'll set r+, you can resolve the remaining issues locally.

> Source/WebCore/svg/SVGAnimateElement.cpp:133
> +    if (type == AnimatedUnknown)
> +        return AnimatedUnknown;
> +
> +    if (hasTagName(SVGNames::animateColorTag) && type != AnimatedColor)

Maybe combine into one?

> Source/WebCore/svg/SVGAnimateElement.cpp:153
> +    if (type == AnimatedBoolean
> +        || type == AnimatedEnumeration
> +        || type == AnimatedLengthList
> +        || type == AnimatedNumberList
> +        || type == AnimatedNumberOptionalNumber
> +        || type == AnimatedPreserveAspectRatio
> +        || type == AnimatedRect)
> +        return AnimatedString;
> +    if (type == AnimatedAngle
> +        || type == AnimatedInteger
> +        || type == AnimatedLength)
> +        return AnimatedNumber;
> +    // Animations of transform lists are not allowed for <animate> or <set>
> +    // http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties
> +    if (type == AnimatedTransformList)
> +        return AnimatedUnknown;

I'd love to see a switch() here, w/o a default: implementation, so we can  be sure we always handle all possible property types here.

> Source/WebCore/svg/SVGAnimateElement.cpp:190
> -            adjustForInheritance(targetElement, attributeName(), fromNumberString);
> +            adjustForInheritance(targetElement, attributeName().localName(), fromNumberString);

How about making adjustForInheritance take a QName&, that would remove the need to add .localName() in several places.

> Source/WebCore/svg/SVGAnimateElement.cpp:340
> +    if (m_animatedAttributeType == AnimatedColor) {

This function would be so much easier, if we had a global switch here, that would call into the various helper methods, to handle AnimateColor, AnimatedNumber etc..
But this would be another cleanup patch I think..

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:158
> +    AffineTransform* transform = targetElement()->supplementalTransform();

No need to check for targetElement() here, but in calculateAnimatedValue below, why?

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:189
> +    if (targetElement->renderer())

if (RenderObject* targetRenderer = ...)
    targetRenderer->setNeeds..

> Source/WebCore/svg/animation/SVGSMILElement.cpp:144
> +static inline QualifiedName constructQualifiedName(const SVGElement* svgElement, const String& attributeName)

Great!

[Dirk Schulze]

Committed r79155: <http://trac.webkit.org/changeset/79155>