[webkit-reviews] review granted: [Bug 217707] Simplify management of LayerPropertyAnimation instances in GraphicsLayerCA : [Attachment 411344] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 217707: Simplify management of LayerPropertyAnimation instances in
GraphicsLayerCA
https://bugs.webkit.org/show_bug.cgi?id=217707

Attachment 411344: Patch

https://bugs.webkit.org/attachment.cgi?id=411344&action=review




--- Comment #10 from Dean Jackson <dino at apple.com> ---
Comment on attachment 411344
  --> https://bugs.webkit.org/attachment.cgi?id=411344
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:700
> +	   if ((animation.m_property == AnimatedPropertyTransform
> +	       || animation.m_property == AnimatedPropertyOpacity
> +	       || animation.m_property == AnimatedPropertyBackgroundColor
> +	       || animation.m_property == AnimatedPropertyFilter)

Could these be any other properties? Do we need to test?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:4226
> +	   auto caAnimation =
animatedLayer(animation.m_property)->animationForKey(animationIdentifier(animat
ion.m_name, animation.m_property, animation.m_index, animation.m_subIndex));

make an animationIdentifier() override that takes an animation rather than
having to list all four properties each time. You still need the one with four
parameters for the case where you don't have an animation object, but a single
argument method would be nice.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:470
> +    enum PlayState { Playing, PlayPending, Paused, PausePending };

enum class

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:486
>	   Seconds m_timeOffset;

Make this { 0_s } too.