Bug 218080 – REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window

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

[Antoine Quint]

REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window

[Antoine Quint]

Created attachment 412100 [details]
Patch

[Antoine Quint]

<rdar://problem/70547132>

[Simon Fraser (smfr)]

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.

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 412183 [details]
Patch

[Dean Jackson]

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?

[Antoine Quint]

Committed r268932: <https://trac.webkit.org/changeset/268932>

[Antoine Quint]

(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.