Bug 218080 - REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window
Summary: REGRESSION(r268615): certain animations break when moving from one to display...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-22 08:33 PDT by Antoine Quint
Modified: 2021-08-05 11:06 PDT (History)
15 users (show)

See Also:

Attachments
Patch (2.07 KB, patch)
2020-10-22 08:39 PDT, Antoine Quint 		no flags Details | Formatted Diff | Diff
Patch (26.50 KB, patch)
2020-10-23 08:29 PDT, Antoine Quint 		dino: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-10-22 08:33:04 PDT 
REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window
Comment 1 Antoine Quint 2020-10-22 08:39:24 PDT 
Created attachment 412100 [details]
Patch
Comment 2 Antoine Quint 2020-10-22 08:39:29 PDT 
<rdar://problem/70547132>
Comment 3 Simon Fraser (smfr) 2020-10-22 09:10:07 PDT 
Comment on attachment 412100 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:-669
> -        noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);

Does bug 218081 cover the CoverageRectChanged case? That's about getting the tiling area right for layers which are children of an animating layer.
Comment 4 Antoine Quint 2020-10-22 10:37:35 PDT 
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 412100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412100&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:-669
> > -        noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
> 
> Does bug 218081 cover the CoverageRectChanged case? That's about getting the
> tiling area right for layers which are children of an animating layer.

We should probably file a different bug for this. I'd appreciate some leads related to testing.
Comment 5 Antoine Quint 2020-10-23 08:29:41 PDT 
Created attachment 412183 [details]
Patch
Comment 6 Dean Jackson 2020-10-23 11:30:06 PDT 
Comment on attachment 412183 [details]
Patch

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

> Source/WebCore/style/StyleTreeResolver.cpp:352
> +        auto previousLastStyleChangeEventStyle = styleable.lastStyleChangeEventStyle() ? RenderStyle::clonePtr(*styleable.lastStyleChangeEventStyle()) : RenderStyle::createPtr();

Do you need to clone/copy? Does applyKeyframeEffects mutate the style?
Comment 7 Antoine Quint 2020-10-23 11:35:54 PDT 
Committed r268932: <https://trac.webkit.org/changeset/268932>
Comment 8 Antoine Quint 2020-10-23 11:42:27 PDT 
(In reply to Dean Jackson from comment #6)
> Comment on attachment 412183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412183&action=review
> 
> > Source/WebCore/style/StyleTreeResolver.cpp:352
> > +        auto previousLastStyleChangeEventStyle = styleable.lastStyleChangeEventStyle() ? RenderStyle::clonePtr(*styleable.lastStyleChangeEventStyle()) : RenderStyle::createPtr();
> 
> Do you need to clone/copy? Does applyKeyframeEffects mutate the style?

This is because we immediately call styleable.setLastStyleChangeEventStyle() which would make the previous value no longer owned by anything.