[Webkit-unassigned] [Bug 54542] SVG animation - analyze attribute type for animation

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82848|review?                     |review+
               Flag|                            |




--- Comment #8 from Nikolas Zimmermann <zimmermann at kde.org>  2011-02-19 00:09:06 PST ---
(From update of attachment 82848)
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!

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