Bug 212103

Created attachment 399764 [details]
Patch

<rdar://problem/62737868>

Comment on attachment 399764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399764&action=review

> Source/WebCore/animation/AnimationEffect.cpp:547
> +    if (!is<StepsTimingFunction>(m_timingFunction))
> +        return WTF::nullopt;

Does this get called for animations whose keyframes all have the same value? Or did we optimize those out earlier?

> Source/WebCore/animation/KeyframeEffect.cpp:1825
> +        if (intervalEndProgress <= iterationProgress)
> +            continue;
> +
> +        if (!i)
> +            break;

This is pretty confusing. The !i makes it always break after i=0, but you continue earlier. Easier to grok if it's flipped around somehow.

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 399764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399764&action=review
> 
> > Source/WebCore/animation/AnimationEffect.cpp:547
> > +    if (!is<StepsTimingFunction>(m_timingFunction))
> > +        return WTF::nullopt;
> 
> Does this get called for animations whose keyframes all have the same value?
> Or did we optimize those out earlier?

We don’t optimize this case in any way for now. So this would get called for any animation.

> > Source/WebCore/animation/KeyframeEffect.cpp:1825
> > +        if (intervalEndProgress <= iterationProgress)
> > +            continue;
> > +
> > +        if (!i)
> > +            break;
> 
> This is pretty confusing. The !i makes it always break after i=0, but you
> continue earlier. Easier to grok if it's flipped around somehow.

The order matters here. We just want to be sure that we don’t deal with an implicit 0% keyframe since we wouldn’t have a blending keyframe for that and would otherwise crash trying to access its timing function.

Created attachment 399827 [details]
Patch

Created attachment 399828 [details]
Patch

Created attachment 399835 [details]
Patch

Comment on attachment 399835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399835&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:1476
> +    if (index < 0)
> +        return nullptr;

size_t is unsigned, this can never happen.

> Source/WebCore/animation/KeyframeEffect.cpp:1837
> +Optional<double> KeyframeEffect::progressUntilNextStep(double iterationProgress) const

"progress until" is a bit ambiguous here. Maybe "progress at next step"?

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 399835 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399835&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:1476
> > +    if (index < 0)
> > +        return nullptr;
> 
> size_t is unsigned, this can never happen.

Will remove in commit.

> > Source/WebCore/animation/KeyframeEffect.cpp:1837
> > +Optional<double> KeyframeEffect::progressUntilNextStep(double iterationProgress) const
> 
> "progress until" is a bit ambiguous here. Maybe "progress at next step"?

"progress at next step" would not be correct, what we want to know is "how much progress until the next step". I think within the context of determining the amount of time before the next step, which is the only scenario in which this method is used, this is clear enough.

Committed r261926: <https://trac.webkit.org/changeset/261926>

Committed r261927: <https://trac.webkit.org/changeset/261927>

*** Bug 45447 has been marked as a duplicate of this bug. ***