[webkit-reviews] review denied: [Bug 45325] SVG Animate / Animate transform not animating after change via JS API : [Attachment 72809] Add test case. V2.

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Robin Qiu
<robin.qiu at torchmobile.com.cn>'s request for review:
Bug 45325: SVG Animate / Animate transform not animating after change via JS
API
https://bugs.webkit.org/show_bug.cgi?id=45325

Attachment 72809: Add test case. V2.
https://bugs.webkit.org/attachment.cgi?id=72809&action=review

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

> LayoutTests/svg/animations/script-tests/animate-set-attribute-frozen.js:1
> +description("Animation should not be frozen after setAttrubite() and the new
attribute should be applied.");

Typo: setAttribute.

> LayoutTests/svg/animations/script-tests/animate-set-attribute-frozen.js:30
> +// Setup animation test
> +function expectDarkBlue() {
> +    // Check half-time conditions
> +    shouldBeFalse("rect.style.fill == '#0000FF'");
> +    shouldBe("rect.style.fill.toString().substring(0,5)", "'#0000'");
> +    completeTest();
> +}
> +
> +window.setTimeout("triggerUpdate(50, 50)", 0);
> +window.setTimeout('animate.setAttribute("to", "#000000");', 1000);
> +window.setTimeout('expectDarkBlue()', 1500);

This is not correct.
You should use the snapshot API, which is capable of giving you the current DOM
at a specified time.
See LayoutTests/svg/animations/script-tests/animate-color-transparent.js as
example.

I'd add an "prepareTest()" method, which does:
function prepareTest() {
    animate.setAttribute("to", "#00000");
    window.setTimeout(executeTest, 0);
}
and use
// Begin test async
rect.setAttribute("onclick", "prepareTest()");

instead of

// Begin test async
rect.setAttribute("onclick", "executeTest()");

That should give you a reliable way of testing this, which is not timing
dependant.