[webkit-reviews] review denied: [Bug 43452] SVG animation beginElement() does not restart a stopped animation. : [Attachment 95170] Bugfix for SVG animation restarting. Updated Patch.

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Felician Marton
<marton.felician.zoltan at stud.u-szeged.hu>'s request for review:
Bug 43452: SVG animation beginElement() does not restart a stopped animation.
https://bugs.webkit.org/show_bug.cgi?id=43452

Attachment 95170: Bugfix for SVG animation restarting. Updated Patch.
https://bugs.webkit.org/attachment.cgi?id=95170&action=review

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

Test is still flakey, needs another iteration: r-:

> LayoutTests/ChangeLog:7
> +	   https://bugs.webkit.org/show_bug.cgi?id=43452
> +	   Added test for animation beginElement. It should restart the
animation even if the animation is stopped by endElement() previously.

You should leave a blank line below the bug link.

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:10

> +rect.setAttribute("fill", "pink");

Please use green here not pink ;-)

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:43

> +    //Timeout needed, because te SVG animation mechanism is fully time
based.
> +    //If animation actions performed in a very short time, the actions will
not behave as expected.
> +    setTimeout("animateX.beginElement()",30);
> +    setTimeout("animateX.endElement()",60);
> +    setTimeout("animateX.beginElement(), checkPosition()",90);

This hack is not acceptable, it makes the test timing dependant, and likely
flakey. What you want to use is:

function beginAnimation() {
    animateX.beginElement();
    setTimeout(restartAnimation, 0);
}

function restartAnimation() {
    animateX.endElement();
    setTimeout(function() {
	animateX.beginElement();
	checkPosition();
    }, 0);
}

function executeTest() {
    setTimeout(beginElement, 0);
}

That looks cleaner, right?

> Source/WebCore/ChangeLog:5
> +	   Bugfix: SVG animation beginElement() does not restart the animation
after endElement().

This is not the real bug title, it should always match the bugzilla title, so
remove the "Bugfix: ".

> Source/WebCore/ChangeLog:9
> +

You should describe here what actually changed. Sth like:
Calling beginElement() after calling endElement() does not restart the
animation.