[webkit-reviews] review denied: [Bug 63911] REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits assertion failure. : [Attachment 99648] 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 63911: REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits
assertion failure.
https://bugs.webkit.org/show_bug.cgi?id=63911

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

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

This is a regression, so we need a regression test. Can you add a test to
svg/animation please? I also have a question to your fix. r- because of the
missing test.

> Source/WebCore/svg/SVGAnimationElement.cpp:494
> +    if (percent == 1) {
> +	   from = m_values[valuesCount - 1];
> +	   to = m_values[valuesCount - 1];
> +	   effectivePercent = 1;
> +	   return;
> +    }

I just wonder. Here we return always effectivePercent = 1 if percent is 1,...

> Source/WebCore/svg/SVGAnimationElement.cpp:520
>      if (calcMode == CalcModeDiscrete) {
>	   if (!keyTimesCount) 
> -	       index = percent == 1 ? valuesCount - 1 :
static_cast<unsigned>(percent * valuesCount);
> +	       index = static_cast<unsigned>(percent * valuesCount);
>	   from = m_values[index];
>	   to = m_values[index];
>	   effectivePercent = 0;

here we return effectivePercent = 0 for discrete animations. Sure that this
doesn't cause new problems?