Bug 217707 – Simplify management of LayerPropertyAnimation instances in GraphicsLayerCA

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

Simplify management of LayerPropertyAnimation instances in GraphicsLayerCA

[Radar WebKit Bug Importer]

<rdar://problem/70291140>

[Antoine Quint]

Created attachment 411320 [details]
Patch

[Antoine Quint]

<rdar://problem/70291140>

[Simon Fraser (smfr)]

Comment on attachment 411320 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:701
> +            && animation.m_playState == Playing)

I think we'd need to copy paused animations too

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 411320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411320&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:701
> > +            && animation.m_playState == Playing)
> 
> I think we'd need to copy paused animations too

Oh yeah, good point! We need to copy any committed animation.

[Antoine Quint]

Created attachment 411341 [details]
Patch

[Antoine Quint]

Created attachment 411342 [details]
Patch

[Antoine Quint]

Created attachment 411344 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411344 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2885
> -    if (timeOffset)
> -        caAnim.setBeginTime(CACurrentMediaTime() - timeOffset.seconds());
> +    caAnim.setBeginTime(beginTime.seconds());

Why this change? You're changing behavior from CA implicitly setting the beginTime when it commits the animation to explicitly setting beginTime.

We still have code that seems to care about this: see animationDidStart:

[Dean Jackson]

Comment on attachment 411344 [details]
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(animation.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.

[Antoine Quint]

Committed r268483: <https://trac.webkit.org/changeset/268483>