Bug 189990 – [Web Animations] Ensure renderers with accelerated animations have layers

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

[Antoine Quint]

[Web Animations] Ensure renderers with accelerated animations have layers

[Antoine Quint]

We have done some work already in https://bugs.webkit.org/show_bug.cgi?id=189784 to prevent never-ending calls to DocumentTimeline::updateAnimations(). This was due to the change made for https://bugs.webkit.org/show_bug.cgi?id=186930 where we queued calls to updateAnimations() in KeyframeEffectReadOnly::applyPendingAcceleratedActions() while we were waiting for a renderer with a layer backing for a given animation target. Instead of doing this, we ought to ensure renderers always have a layer when they have an accelerated animation applied.

[Radar WebKit Bug Importer]

<rdar://problem/44791222>

[Antoine Quint]

Created attachment 350852 [details]
Patch

[EWS Watchlist]

Comment on attachment 350852 [details]
Patch

Attachment 350852 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9354714

New failing tests:
transforms/3d/general/perspective-non-layer.html
fast/table/overflow-table-collapsed-borders-section-self-painting-layer-painting.html
fast/transforms/transform-on-inline.html
fast/reflections/inline-crash.html
fast/table/overflow-table-collapsed-borders-section-self-painting-layer-table-self-painting-layer.html

[EWS Watchlist]

Created attachment 350853 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350852 [details]
Patch

Attachment 350852 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9354760

New failing tests:
transforms/3d/general/perspective-non-layer.html
fast/table/overflow-table-collapsed-borders-section-self-painting-layer-painting.html
fast/transforms/transform-on-inline.html
fast/reflections/inline-crash.html
fast/table/overflow-table-collapsed-borders-section-self-painting-layer-table-self-painting-layer.html

[EWS Watchlist]

Created attachment 350854 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 350856 [details]
Patch

[zalan]

Comment on attachment 350856 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2697
> +            return timeline->runningAnimationsForElementAreAllAccelerated(*node);

I hope this function is not going to show up in traces.

[Antoine Quint]

Committed r236501: <https://trac.webkit.org/changeset/236501>

[Simon Fraser (smfr)]

Comment on attachment 350856 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.h:57
>          return isDocumentElementRenderer() || isPositioned() || createsGroup() || hasClipPath() || hasOverflowClip()
>              || hasTransformRelatedProperty() || hasHiddenBackface() || hasReflection() || style().specifiesColumns()
> -            || !style().hasAutoZIndex();
> +            || !style().hasAutoZIndex() || hasRunningAcceleratedAnimations();
>      }

This is pretty hot code. I'm worried about perf impact.

> Source/WebCore/rendering/RenderInline.h:123
> +    bool requiresLayer() const override { return isInFlowPositioned() || createsGroup() || hasClipPath() || willChangeCreatesStackingContext() || hasRunningAcceleratedAnimations(); }

This is pretty hot code. I'm worried about perf impact.

[Antoine Quint]

I suppose we could set a flag on the RenderBoxModelObject to indicate that an accelerated animation is in progress as the accelerated actions are updated on the KeyframeEffectReadOnly. Then it would just be a bit to read on the RenderBoxModelObject.

[Simon Fraser (smfr)]

We used to have a bit on the style, and removed it. I think for a time we also used to have a bit on renderers, and that was removed too (I can't find the patches).

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #13)
> We used to have a bit on the style, and removed it. I think for a time we
> also used to have a bit on renderers, and that was removed too (I can't find
> the patches).

Do you have a preference between the two?

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 350956 [details]
Patch

[zalan]

Comment on attachment 350956 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.h:333
> +    bool m_hasRunningAcceleratedAnimations { false };

smfr, what's our current (memory)policy about adding bitfield like members to RenderObject subclasses? Should this be part of RenderObject's boolean bitfield (or even rare data)?

[Antoine Quint]

Created attachment 351247 [details]
Patch

[Antoine Quint]

Committed r236670: <https://trac.webkit.org/changeset/236670>