Created attachment 448380 [details] Patch

Comment on attachment 448380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448380&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1497 > + std::optional<size_t> firstImplicitValueIndex; > + if (property == CSSPropertyAnimationDuration || property == CSSPropertyTransitionDuration) > + firstImplicitValueIndex = animationList.firstImplicitDurationIndex(); > + else if (property == CSSPropertyAnimationDelay || property == CSSPropertyTransitionDelay) > + firstImplicitValueIndex = animationList.firstImplicitDelayIndex(); > + else if (property == CSSPropertyAnimationTimingFunction || property == CSSPropertyTransitionTimingFunction) > + firstImplicitValueIndex = animationList.firstImplicitTimingFunctionIndex(); > + else if (property == CSSPropertyAnimationDirection) > + firstImplicitValueIndex = animationList.firstImplicitDirectionIndex(); > + else if (property == CSSPropertyAnimationIterationCount) > + firstImplicitValueIndex = animationList.firstImplicitIterationCountIndex(); > + else if (property == CSSPropertyAnimationFillMode) > + firstImplicitValueIndex = animationList.firstImplicitFillModeIndex(); > + else if (property == CSSPropertyAnimationPlayState) > + firstImplicitValueIndex = animationList.firstImplicitPlayStateIndex(); > + else if (property == CSSPropertyTransitionProperty) > + firstImplicitValueIndex = animationList.firstImplicitPropertyIndex(); I'd write this as lambda that uses switch and returns std::optional<size_t> firstImplicitValueIndex Or at least use switch. > Source/WebCore/platform/animation/AnimationList.h:84 > + std::optional<size_t> m_firstImplicitDelayIndex; > + std::optional<size_t> m_firstImplicitDirectionIndex; > + std::optional<size_t> m_firstImplicitDurationIndex; > + std::optional<size_t> m_firstImplicitFillModeIndex; > + std::optional<size_t> m_firstImplicitIterationCountIndex; > + std::optional<size_t> m_firstImplicitPlayStateIndex; > + std::optional<size_t> m_firstImplicitPropertyIndex; > + std::optional<size_t> m_firstImplicitTimingFunctionIndex; This would be pretty wasteful if we cared about AnimationList memory use. But maybe we don't? Why can't this just be a bit in Animation?

Created attachment 448383 [details] Alternative patch

(In reply to Antti Koivisto from comment #2) > Comment on attachment 448380 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448380&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1497 > > + std::optional<size_t> firstImplicitValueIndex; > > + if (property == CSSPropertyAnimationDuration || property == CSSPropertyTransitionDuration) > > + firstImplicitValueIndex = animationList.firstImplicitDurationIndex(); > > + else if (property == CSSPropertyAnimationDelay || property == CSSPropertyTransitionDelay) > > + firstImplicitValueIndex = animationList.firstImplicitDelayIndex(); > > + else if (property == CSSPropertyAnimationTimingFunction || property == CSSPropertyTransitionTimingFunction) > > + firstImplicitValueIndex = animationList.firstImplicitTimingFunctionIndex(); > > + else if (property == CSSPropertyAnimationDirection) > > + firstImplicitValueIndex = animationList.firstImplicitDirectionIndex(); > > + else if (property == CSSPropertyAnimationIterationCount) > > + firstImplicitValueIndex = animationList.firstImplicitIterationCountIndex(); > > + else if (property == CSSPropertyAnimationFillMode) > > + firstImplicitValueIndex = animationList.firstImplicitFillModeIndex(); > > + else if (property == CSSPropertyAnimationPlayState) > > + firstImplicitValueIndex = animationList.firstImplicitPlayStateIndex(); > > + else if (property == CSSPropertyTransitionProperty) > > + firstImplicitValueIndex = animationList.firstImplicitPropertyIndex(); > > I'd write this as lambda that uses switch and returns std::optional<size_t> > firstImplicitValueIndex > > Or at least use switch. Lambda + switch would be much better, agreed. If we go with this patch, I'll make that change. > > Source/WebCore/platform/animation/AnimationList.h:84 > > + std::optional<size_t> m_firstImplicitDelayIndex; > > + std::optional<size_t> m_firstImplicitDirectionIndex; > > + std::optional<size_t> m_firstImplicitDurationIndex; > > + std::optional<size_t> m_firstImplicitFillModeIndex; > > + std::optional<size_t> m_firstImplicitIterationCountIndex; > > + std::optional<size_t> m_firstImplicitPlayStateIndex; > > + std::optional<size_t> m_firstImplicitPropertyIndex; > > + std::optional<size_t> m_firstImplicitTimingFunctionIndex; > > This would be pretty wasteful if we cared about AnimationList memory use. > But maybe we don't? > > Why can't this just be a bit in Animation? I felt like knowing which of the values were filled was more of a concern of AnimationList than Animation items themselves, and also we'd only need to store one but per list rather than on each Animation. I did however make an alternative patch which stores this information on Animation, so you can see if you think this is better or worse.

Comment on attachment 448383 [details] Alternative patch View in context: https://bugs.webkit.org/attachment.cgi?id=448383&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1448 > + if (property == CSSPropertyAnimationDuration || property == CSSPropertyTransitionDuration) { switch? > Source/WebCore/platform/animation/Animation.cpp:93 > , m_timingFunctionSet(o.m_timingFunctionSet) > , m_compositeOperationSet(o.m_compositeOperationSet) > , m_isNone(o.m_isNone) > + , m_delayFilled(o.m_delayFilled) > + , m_directionFilled(o.m_directionFilled) > + , m_durationFilled(o.m_durationFilled) > + , m_fillModeFilled(o.m_fillModeFilled) > + , m_iterationCountFilled(o.m_iterationCountFilled) > + , m_playStateFilled(o.m_playStateFilled) > + , m_propertyFilled(o.m_propertyFilled) > + , m_timingFunctionFilled(o.m_timingFunctionFilled) Does this really need an explicit copy constructor? Doesn't Animation::Animation(const Animation&) = default; work?

(In reply to Antti Koivisto from comment #5) > Comment on attachment 448383 [details] > Alternative patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448383&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1448 > > + if (property == CSSPropertyAnimationDuration || property == CSSPropertyTransitionDuration) { > > switch? Not a fan of a switch in this case, it ends up being messy because ideally it would return a CSSValue, but we wouldn't return on in all control paths, which means either fallthroughs or repeated return statements. I'd rather leave it as-is for now. > > Source/WebCore/platform/animation/Animation.cpp:93 > > , m_timingFunctionSet(o.m_timingFunctionSet) > > , m_compositeOperationSet(o.m_compositeOperationSet) > > , m_isNone(o.m_isNone) > > + , m_delayFilled(o.m_delayFilled) > > + , m_directionFilled(o.m_directionFilled) > > + , m_durationFilled(o.m_durationFilled) > > + , m_fillModeFilled(o.m_fillModeFilled) > > + , m_iterationCountFilled(o.m_iterationCountFilled) > > + , m_playStateFilled(o.m_playStateFilled) > > + , m_propertyFilled(o.m_propertyFilled) > > + , m_timingFunctionFilled(o.m_timingFunctionFilled) > > Does this really need an explicit copy constructor? Doesn't > > Animation::Animation(const Animation&) = default; > > work? It doesn't *just* work, I get this error: error: explicitly defaulted copy constructor is implicitly deleted [-Werror,-Wdefaulted-function-deleted] Animation(const Animation&) = default;

Committed r287678 (245775@trunk): <https://commits.webkit.org/245775@trunk>

<rdar://problem/87189586>