[Web Animations] Animation engine should not wake up every tick for steps timing functions

[Antoine Quint]

[Web Animations] Animation engine should not wake up every tick for steps timing functions

[Antoine Quint]

Created attachment 399764 [details]
Patch

[Antoine Quint]

<rdar://problem/62737868>

[Simon Fraser (smfr)]

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.

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 399827 [details]
Patch

[Antoine Quint]

Created attachment 399828 [details]
Patch

[Antoine Quint]

Created attachment 399835 [details]
Patch

[Simon Fraser (smfr)]

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"?

[Antoine Quint]

(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.

[Antoine Quint]

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

[Antoine Quint]

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