[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 #82691|review?                     |review-
               Flag|                            |




--- Comment #6 from Nikolas Zimmermann <zimmermann at kde.org>  2011-02-17 00:21:22 PST ---
(From update of attachment 82691)
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;

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