Created attachment 132457 [details] Patch

Comment on attachment 132457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132457&action=review > Source/WebCore/svg/SVGAnimateElement.cpp:245 > + for (size_t i = 0; i < m_animatedProperties.size(); ++i) > + ASSERT(m_animatedPropertyType == m_animatedProperties[i]->animatedPropertyType()); You should add an inline function returning bool for this and then assert on that, ASSERT(propertyTypesConsistent()) or similar. > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:74 > + AnimatedType* copy = new AnimatedType(*reinterpret_cast<AnimatedType*>(animatedType)); > + types.append(reinterpret_cast<SVGGenericAnimatedType*>(copy)); Why is this reinterpret_cast needed? Why is it safe? There should be at least a comment. > Source/WebCore/svg/SVGAnimationElement.cpp:383 > + size_t propertiesSize = properties.size(); > + if (propertiesSize) { Note that we want to make vectors use 32 bit indexes in the future so the general direction is away from size_t. But probably better use it here for consistency for now. > Source/WebCore/svg/SVGAnimationElement.cpp:480 > -void SVGAnimationElement::animationStarted(SVGAnimatedProperty* property, SVGAnimatedType* type) > +void SVGAnimationElement::animationStarted(const Vector<SVGAnimatedProperty*>& properties, const Vector<SVGGenericAnimatedType*>& types) > { > - ASSERT(type); > - notifyAnimatedPropertyAboutAnimationBeginEnd(property, type, targetElement(), attributeName()); > + notifyAnimatedPropertyAboutAnimationBeginEnd(properties, types, targetElement(), attributeName()); It might be good to add a comment explaining what exactly is the case with more than one property per element.

(In reply to comment #2) > (From update of attachment 132457 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132457&action=review > > > Source/WebCore/svg/SVGAnimateElement.cpp:245 > > + for (size_t i = 0; i < m_animatedProperties.size(); ++i) > > + ASSERT(m_animatedPropertyType == m_animatedProperties[i]->animatedPropertyType()); > > You should add an inline function returning bool for this and then assert on that, ASSERT(propertyTypesConsistent()) or similar. Good idea. > > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:74 > Why is this reinterpret_cast needed? Why is it safe? SVGGenericAnimatedType isn't defined anywhere, we use it as generic variant type. This patch is a first step moving away from that, to avoid reinterpret_casts in future. > There should be at least a comment. Ok. > > Source/WebCore/svg/SVGAnimationElement.cpp:383 > Note that we want to make vectors use 32 bit indexes in the future so the general direction is away from size_t. But probably better use it here for consistency for now. Ok, good to know. > > Source/WebCore/svg/SVGAnimationElement.cpp:480 > It might be good to add a comment explaining what exactly is the case with more than one property per element. Ok.

Comment on attachment 132457 [details] Patch r=me since this is an existing architecture. The code that requires reinterpret_casts here absolutely need to be eliminated soon and replaced with properly designed types. The code is completely unsafe as is.

(In reply to comment #4) > (From update of attachment 132457 [details]) > r=me since this is an existing architecture. The code that requires reinterpret_casts here absolutely need to be eliminated soon and replaced with properly designed types. The code is completely unsafe as is. Thanks, I'll post the follow-up patch soon.

Committed r111120: <http://trac.webkit.org/changeset/111120>