Bug 214712

Summary: steps() timing function on a transform animation triggers a render every frame
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: AnimationsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, ggaren, graouts, graouts, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=214708
https://bugs.webkit.org/show_bug.cgi?id=215229
https://bugs.webkit.org/show_bug.cgi?id=215853
Attachments:
Description Flags
WIP
none
Patch thorton: review+

Description Simon Fraser (smfr) 2020-07-23 16:54:14 PDT
steps() timing function on a transform animation triggers a render every frame
Comment 1 Simon Fraser (smfr) 2020-07-23 16:54:42 PDT
Created attachment 405097 [details]
WIP
Comment 2 Simon Fraser (smfr) 2020-07-23 16:54:45 PDT
<rdar://problem/62737868>
Comment 3 Simon Fraser (smfr) 2020-07-23 22:46:23 PDT
Created attachment 405129 [details]
Patch
Comment 4 Sam Weinig 2020-07-24 08:49:55 PDT
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.
Comment 5 Tim Horton 2020-07-24 10:04:32 PDT
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?
Comment 6 Darin Adler 2020-07-24 11:14:13 PDT
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".
Comment 7 Geoffrey Garen 2020-07-24 11:30:13 PDT
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.
Comment 8 Darin Adler 2020-07-24 11:39:06 PDT
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.
Comment 9 Simon Fraser (smfr) 2020-07-24 12:17:56 PDT
(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".
Comment 10 Simon Fraser (smfr) 2020-07-24 14:04:28 PDT
https://trac.webkit.org/changeset/264856/webkit
Comment 11 Antoine Quint 2020-08-27 07:58:23 PDT
This caused bug 215853.