Bug 54542

Created attachment 82632 [details] Patch

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!

Created attachment 82684 [details] Patch

(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.

Created attachment 82691 [details] Patch

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;

Created attachment 82848 [details] Patch

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!

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