steps() timing function on a transform animation triggers a render every frame
Created attachment 405097 [details] WIP
<rdar://problem/62737868>
Created attachment 405129 [details] Patch
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 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 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 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 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.
(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".
https://trac.webkit.org/changeset/264856/webkit
This caused bug 215853.