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+

Antoine Quint
Reported 2020-10-14 07:04:04 PDT
Simplify management of LayerPropertyAnimation instances in GraphicsLayerCA
Attachments
Patch (22.48 KB, patch)
2020-10-14 07:14 PDT, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (23.40 KB, patch)
2020-10-14 10:04 PDT, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (23.39 KB, patch)
2020-10-14 10:05 PDT, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (23.39 KB, patch)
2020-10-14 10:08 PDT, Antoine Quint 		dino: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-10-14 07:06:07 PDT
<rdar://problem/70291140>
Antoine Quint
Comment 2 2020-10-14 07:14:12 PDT
Created attachment 411320 [details] Patch
Antoine Quint
Comment 3 2020-10-14 07:14:17 PDT
<rdar://problem/70291140>
Simon Fraser (smfr)
Comment 4 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
Antoine Quint
Comment 5 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.
Antoine Quint
Comment 6 2020-10-14 10:04:26 PDT
Created attachment 411341 [details] Patch
Antoine Quint
Comment 7 2020-10-14 10:05:53 PDT
Created attachment 411342 [details] Patch
Antoine Quint
Comment 8 2020-10-14 10:08:57 PDT
Created attachment 411344 [details] Patch
Simon Fraser (smfr)
Comment 9 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:
Dean Jackson
Comment 10 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.
Antoine Quint
Comment 11 2020-10-14 13:38:03 PDT
Committed r268483: <https://trac.webkit.org/changeset/268483>
Note You need to log in before you can comment on or make changes to this bug.