[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 101117] 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 101117: Patch
https://bugs.webkit.org/attachment.cgi?id=101117&action=review

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


Sorry for the delay. I'm a bit busy at the moment. r- to get it from pending
review list. Needs more discussion.

> Source/WebCore/ChangeLog:10
> +	   elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd
can be before
> +	   the end of the animation. (e.g. begin="0s" end="2s" dur="3s")

I'm confused about this interpretation. For me the ending of the interval is at
3s, even if we end the animation before that time. What about this example:
<rect width="0" height="100">
    <animate attributeName="width" from="0" to="300" begin="0s" dur="3s"
end="2s" repeat="3"/>
</rect>

With your interpretation of interval end we would repeat the animation after
2s, not after 3s. I'd interprete the animation with start every 3 seconds but
animate just 2 seconds. Maybe I just misunderstand your intention.

> Source/WebCore/ChangeLog:14
> +	   This change makes the function return 100% only when
> +	   m_intervalEnd - m_intervalBegin == simpleDuration, which means the
animation stops
> +	   at the end of the animation. This also does some refactoring to
remove unnecessary

this sounds more reasonable.

> Source/WebCore/ChangeLog:17
> +	   m_intervalEnd - m_intervalBegin. I believe this is more intuitive
because the animation
> +	   cannot progress beyond it.

Can you add an ASSERT if the animation shouldn't proceed beyond that time?

> Source/WebCore/svg/animation/SVGSMILElement.cpp:826
> +    SMILTime activeTime = min(elapsed, m_intervalEnd) - m_intervalBegin;

Still unsure about this.The variable is called activeTime. Is intervalBegin the
relative time from the current repeat level or the absolute time like
elapsedTime? It must be the abs time, otherwise it wouldn't make sense anyway.
And then activeTime would be the relative time from the current repeated
animation. Anyway why do you take intervalEnd if elapsedTime is bigger than
elapsedTime? We should differ between the end of animation process (when end=..
is set) and the end of the current interval. Otherwise the example above would
repeat every 2s instead of every 3s.