Summary: | steps() timing function on a transform animation triggers a render every frame | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | Animations | Assignee: | 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
Simon Fraser (smfr)
2020-07-23 16:54:14 PDT
Created attachment 405097 [details]
WIP
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". This caused bug 215853. |