[webkit-reviews] review denied: [Bug 63246] Convert SVGColor to SVGAnimatorFactory concept : [Attachment 98344] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 63246: Convert SVGColor to SVGAnimatorFactory concept
https://bugs.webkit.org/show_bug.cgi?id=63246

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98344&action=review

I agree the style bot shouldn't warn here - the code should be safe as-is.
Great patch, still some tweaks needed IMHO to make it perfect:

> Source/WebCore/ChangeLog:12
> +	   This information is accessible with the animation element. The
animators store the pointer of the animation element, so that it is not

This information is already exposed by the animation element.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:27
> +SVGAnimatedAngleAnimator::SVGAnimatedAngleAnimator(SVGSMILElement*
animationElement, SVGElement* contextElement)

Why pass in SVGSMILElement and do ....

> Source/WebCore/svg/SVGAnimatedAngle.cpp:54
> +    SVGAnimateElement* animationElement =
static_cast<SVGAnimateElement*>(m_animationElement);

.... static_casts to SVGAnimateElement in the other methods? That seems like a
flaw. Just use SVGAnimateElement in first place, no?

> Source/WebCore/svg/SVGAnimatedNumber.h:46
> +    SVGAnimatedNumberAnimator(SVGSMILElement* animationElement,
SVGElement*);

No need to name the parameter.