[webkit-reviews] review denied: [Bug 63553] SVG animation fill="freeze" doesn't set baseVal to current animVal if animation stops before reaching the end : [Attachment 100227] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Young Han Lee
<joybro at company100.net>'s request for review:
Bug 63553: SVG animation fill="freeze" doesn't set baseVal to current animVal
if animation stops before reaching the end
https://bugs.webkit.org/show_bug.cgi?id=63553

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100227&action=review


I'm confused about your ChangeLog entry since it is the same entry like on
another bug. r- because of the layouttest.

> LayoutTests/svg/animations/script-tests/animate-end-attribute.js:33
> +    ok = isCloseEnough(rect.x.baseVal.value, 200, 0.2);
> +    if (ok)
> +	   testPassed("rect.x.baseVal.value is almost 200");
> +    else
> +	   testFailed("rect.x.baseVal.value is NOT almost 200, as expected");

Please don't do it manually we have a function called shouldBeCloseEnough()
with three arguments. Take a look at other tests.

> LayoutTests/svg/animations/script-tests/animate-end-attribute.js:41
> +    ok = isCloseEnough(rect.x.baseVal.value, 200, 0.2);
> +    if (ok)
> +	   testPassed("rect.x.baseVal.value is almost 200");
> +    else
> +	   testFailed("rect.x.baseVal.value is NOT almost 200, as expected");

Ditto.

> LayoutTests/svg/animations/script-tests/animate-end-attribute.js:49
> +	   ["animation", 2.0,	 "rect", sample2],
> +	   ["animation", 3.0,	 "rect", sample3]

You can reuse sample3 for 2s. Can you please add an animation step at 0s to
verify that we start from baseVal?

> Source/WebCore/ChangeLog:9
> +	   calculateAnimationPercentAndRepeat() is returning 1, which means
100%, whenever
> +	   elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd
can be before

Hm, wasn't it fixed with bug 63911?

> Source/WebCore/svg/animation/SVGSMILElement.cpp:833
> +	   return 1.f;

just use 1, no .f after numbers.