Bug 217707

Summary: Simplify management of LayerPropertyAnimation instances in GraphicsLayerCA
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 178117    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch dino: review+

Description Antoine Quint 2020-10-14 07:04:04 PDT
Simplify management of LayerPropertyAnimation instances in GraphicsLayerCA
Comment 1 Radar WebKit Bug Importer 2020-10-14 07:06:07 PDT
<rdar://problem/70291140>
Comment 2 Antoine Quint 2020-10-14 07:14:12 PDT
Created attachment 411320 [details]
Patch
Comment 3 Antoine Quint 2020-10-14 07:14:17 PDT
<rdar://problem/70291140>
Comment 4 Simon Fraser (smfr) 2020-10-14 08:42:16 PDT
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
Comment 5 Antoine Quint 2020-10-14 08:55:31 PDT
(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.
Comment 6 Antoine Quint 2020-10-14 10:04:26 PDT
Created attachment 411341 [details]
Patch
Comment 7 Antoine Quint 2020-10-14 10:05:53 PDT
Created attachment 411342 [details]
Patch
Comment 8 Antoine Quint 2020-10-14 10:08:57 PDT
Created attachment 411344 [details]
Patch
Comment 9 Simon Fraser (smfr) 2020-10-14 11:55:23 PDT
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:
Comment 10 Dean Jackson 2020-10-14 11:56:58 PDT
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.
Comment 11 Antoine Quint 2020-10-14 13:38:03 PDT
Committed r268483: <https://trac.webkit.org/changeset/268483>