[webkit-reviews] review denied: [Bug 12073] Implement setCurrentTime() and pauseAnimations() on SVGSVGElement : [Attachment 67423] Patch to fix this bug.

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Chris Meyer
<cmeyer1969+webkit at gmail.com>'s request for review:
Bug 12073: Implement setCurrentTime() and pauseAnimations() on SVGSVGElement
https://bugs.webkit.org/show_bug.cgi?id=12073

Attachment 67423: Patch to fix this bug.
https://bugs.webkit.org/attachment.cgi?id=67423&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Chris,

> +void SMILTimeContainer::setElapsed(SMILTime time)
> +{
> +    m_beginTime = currentTime() - time.value();
> +    m_accumulatedPauseTime = 0;
> +    if (isPaused()) {
> +	   SMILTime elapsed = this->elapsed();
> +	   updateAnimations(elapsed);
> +    }
No need for a local variable, just use:
if (isPaused())
    updateAnimations(elapsed());

You have to write tests and a ChangeLog, please use prepare-ChangeLog in your
root WebKit directory to generate templates. r-, until these issues are
resolved. The patch itself looks just fine, but you'll need to write several
testcases, ideally using the existing framework in LayoutTests/svg/animations.
Test it using accumulative animations, normal ones, etc.

Thanks in advance!