[webkit-reviews] review denied: [Bug 63387] SVGAnimatedType should support SVGNumberList animation : [Attachment 98603] 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 63387: SVGAnimatedType should support SVGNumberList animation
https://bugs.webkit.org/show_bug.cgi?id=63387

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

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

Can you please upload a new version that applies so we can check the build?
Setting r- for now.

> Source/WebCore/ChangeLog:16154
> + Ditto.

Typo.

> Source/WebCore/svg/SVGAnimatedNumberList.cpp:67
> +	   toNumberList.at(i) += fromNumberList.at(i);

Use operator{} here.

> Source/WebCore/svg/SVGAnimatedNumberList.cpp:78
> +    ASSERT(m_contextElement);
> +    SVGAnimateElement* animationElement =
static_cast<SVGAnimateElement*>(m_animationElement);
> +    
> +    AnimationMode animationMode = animationElement->animationMode();
> +    // To animation uses contributions from the lower priority animations as
the base value.

I'd prefer:
ASSERT(...);

SVGAnimateElement* animationElement = ..
AnimationMode animationMode = ..

// To animation uses...

> Source/WebCore/svg/SVGAnimatedNumberList.cpp:96
> +	   SVGAnimatedNumberAnimator::calculateAnimatedNumber(animationElement,
percentage, repeatCount, animatedNumberList.at(i), fromNumberList.at(i),
toNumberList.at(i));

Use operator[] instead of at, that's preferred.

> Source/WebCore/svg/SVGAnimatedNumberList.cpp:101
> +    return -1;

Comment?