[webkit-reviews] review granted: [Bug 54542] SVG animation - analyze attribute type for animation : [Attachment 82848] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Feb 19 00:09:05 PST 2011


Nikolas Zimmermann <zimmermann at kde.org> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 54542: SVG animation - analyze attribute type for animation
https://bugs.webkit.org/show_bug.cgi?id=54542

Attachment 82848: Patch
https://bugs.webkit.org/attachment.cgi?id=82848&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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!


More information about the webkit-reviews mailing list