[webkit-reviews] review denied: [Bug 108184] ASSERT(time.isFinite()) in SVGSMILElement::createInstanceTimesFromSyncbase : [Attachment 221344] patch with test

[bugzilla-daemon at webkit.org]

Philip Rogers <pdr at google.com> has denied Tamas Gergely
<tgergely.u-szeged at partner.samsung.com>'s request for review:
Bug 108184: ASSERT(time.isFinite()) in
SVGSMILElement::createInstanceTimesFromSyncbase
https://bugs.webkit.org/show_bug.cgi?id=108184

Attachment 221344: patch with test
https://bugs.webkit.org/attachment.cgi?id=221344&action=review

------- Additional Comments from Philip Rogers <pdr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221344&action=review


Almost there! Just a few minor issues with the test.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:13
> +	   if (window.testRunner) {

I'm confused... this will fire and stop the test before the settimeouts below
even trigger.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:18
> +	   setTimeout('displayMessage()', 500);

This timeout is much too large. Can you try either 0 or, worst case, 30-40? You
can use document.documentElement.setCurrentTime to set the current time so you
don't have to wait for the animation to advance.

We have 20,000 tests and we try very hard to keep them as fast as possible.
Because this test will exist on the tree for years to come, it's important to
ensure it's as fast as we can make it.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:21
> +	       setTimeout('testRunner.notifyDone()', 600);

Same here