Bug 214712 – steps() timing function on a transform animation triggers a render every frame

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Simon Fraser (smfr)]

steps() timing function on a transform animation triggers a render every frame

[Simon Fraser (smfr)]

Created attachment 405097 [details]
WIP

[Simon Fraser (smfr)]

<rdar://problem/62737868>

[Simon Fraser (smfr)]

Created attachment 405129 [details]
Patch

[Sam Weinig]

Comment on attachment 405129 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.h:214
> +    Optional<bool> m_isRunningAccelerated;

I'm really not a fan of using Optional<bool> (outside of a something decoding a bool value or similar uses). I almost never understand what it is trying to represent. In most cases an enum is much clearer. I think that would be true here too.

[Tim Horton]

Comment on attachment 405129 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1558
> +    computeSomeKeyframesUseStepsTimingFunction();

what an interesting function name! (but not new)

>> Source/WebCore/animation/KeyframeEffect.h:214
>> +    Optional<bool> m_isRunningAccelerated;
> 
> I'm really not a fan of using Optional<bool> (outside of a something decoding a bool value or similar uses). I almost never understand what it is trying to represent. In most cases an enum is much clearer. I think that would be true here too.

You'd rather a yes/no/IDK-yet tristate?

[Darin Adler]

Comment on attachment 405129 [details]
Patch

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

>>> Source/WebCore/animation/KeyframeEffect.h:214
>>> +    Optional<bool> m_isRunningAccelerated;
>> 
>> I'm really not a fan of using Optional<bool> (outside of a something decoding a bool value or similar uses). I almost never understand what it is trying to represent. In most cases an enum is much clearer. I think that would be true here too.
> 
> You'd rather a yes/no/IDK-yet tristate?

I don’t have the same problem with it that Sam does. But just reminding you that we do have <wtf/TriState.h> and of course we can make more enums "until the cows come home".

[Geoffrey Garen]

Comment on attachment 405129 [details]
Patch

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

>>>> Source/WebCore/animation/KeyframeEffect.h:214
>>>> +    Optional<bool> m_isRunningAccelerated;
>>> 
>>> I'm really not a fan of using Optional<bool> (outside of a something decoding a bool value or similar uses). I almost never understand what it is trying to represent. In most cases an enum is much clearer. I think that would be true here too.
>> 
>> You'd rather a yes/no/IDK-yet tristate?
> 
> I don’t have the same problem with it that Sam does. But just reminding you that we do have <wtf/TriState.h> and of course we can make more enums "until the cows come home".

I believe the states are: { NotRunning, RunningUnaccelerated, RunningAccelerated }. IDK-yet / Indeterminate aren't a 100% fit here because we always know our state. Perhaps an enum would make this clearer. Or just two separate data members, m_isRunning and m_isAccelerated.

[Darin Adler]

Comment on attachment 405129 [details]
Patch

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

>>>>> Source/WebCore/animation/KeyframeEffect.h:214
>>>>> +    Optional<bool> m_isRunningAccelerated;
>>>> 
>>>> I'm really not a fan of using Optional<bool> (outside of a something decoding a bool value or similar uses). I almost never understand what it is trying to represent. In most cases an enum is much clearer. I think that would be true here too.
>>> 
>>> You'd rather a yes/no/IDK-yet tristate?
>> 
>> I don’t have the same problem with it that Sam does. But just reminding you that we do have <wtf/TriState.h> and of course we can make more enums "until the cows come home".
> 
> I believe the states are: { NotRunning, RunningUnaccelerated, RunningAccelerated }. IDK-yet / Indeterminate aren't a 100% fit here because we always know our state. Perhaps an enum would make this clearer. Or just two separate data members, m_isRunning and m_isAccelerated.

The code definitely implies that WTF::nullopt is used as an "I don’t know" state.

animationDidChangeTimingProperties sets it to WTF::nullopt. That’s not "not running".

I think we just proved that a custom enum would be better than the alternatives above; might help us clarify our thinking.

[Simon Fraser (smfr)]

(In reply to Geoffrey Garen from comment #7)

> I believe the states are: { NotRunning, RunningUnaccelerated,
> RunningAccelerated }. IDK-yet / Indeterminate aren't a 100% fit here because
> we always know our state. Perhaps an enum would make this clearer. Or just
> two separate data members, m_isRunning and m_isAccelerated.

I don't think it includes RunningUnaccelerated (that's surely state that's tracked elsewhere). It's more like "have I tried to run it accelerated: not yet/yes and failed/yes and succeeded".

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/264856/webkit

[Antoine Quint]

This caused bug 215853.