Bug 192604 - [Web Animations] CSSAnimationController should be created lazily
Summary: [Web Animations] CSSAnimationController should be created lazily
Status: RESOLVED WONTFIX
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:
Depends on:
Blocks:
 
Reported: 2018-12-11 14:05 PST by Antoine Quint
Modified: 2020-03-09 13:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (46.41 KB, patch)
2018-12-11 14:09 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.02 MB, application/zip)
2018-12-11 15:30 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.69 MB, application/zip)
2018-12-11 16:00 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (2.53 MB, application/zip)
2018-12-11 16:08 PST, EWS Watchlist
no flags Details
Patch (46.46 KB, patch)
2018-12-12 05:54 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (46.57 KB, patch)
2018-12-12 07:37 PST, Antoine Quint
dino: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (2.33 MB, application/zip)
2018-12-12 09:39 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-12-11 14:05:26 PST
[Web Animations] CSSAnimationController should be created lazily
Comment 1 Antoine Quint 2018-12-11 14:09:09 PST
Created attachment 357074 [details]
Patch
Comment 2 EWS Watchlist 2018-12-11 15:30:32 PST
Comment on attachment 357074 [details]
Patch

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

New failing tests:
transforms/3d/general/prefixed-3dtransform-values.html
media/modern-media-controls/macos-inline-media-controls/macos-inline-media-controls-volume-styles.html
media/modern-media-controls/macos-inline-media-controls/macos-inline-media-controls-start-button-style.html
fast/css/getComputedStyle/computed-style-with-zoom.html
media/modern-media-controls/background-tint/background-tint.html
css3/filters/backdrop/backdropfilter-property-computed-style.html
transforms/3d/general/3dtransform-values.html
Comment 3 EWS Watchlist 2018-12-11 15:30:33 PST
Created attachment 357079 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 4 EWS Watchlist 2018-12-11 16:00:41 PST
Comment on attachment 357074 [details]
Patch

Attachment 357074 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10357912

New failing tests:
fast/css/getComputedStyle/computed-style-with-zoom.html
transforms/3d/general/prefixed-3dtransform-values.html
media/modern-media-controls/background-tint/background-tint.html
css3/filters/backdrop/backdropfilter-property-computed-style.html
transforms/3d/general/3dtransform-values.html
Comment 5 EWS Watchlist 2018-12-11 16:00:43 PST
Created attachment 357082 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 6 EWS Watchlist 2018-12-11 16:08:34 PST
Comment on attachment 357074 [details]
Patch

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

New failing tests:
legacy-animation-engine/imported/blink/compositing/animation/hidden-animated-layer-should-not-have-scrollbars.html
legacy-animation-engine/animations/cross-fade-border-image-source.html
animations/change-keyframes-name.html
legacy-animation-engine/fast/multicol/newmulticol/crash-when-switching-to-floating.html
legacy-animation-engine/imported/blink/animations/wrong-keyframe-name.html
legacy-animation-engine/animations/keyframes.html
inspector/console/webcore-logging.html
legacy-animation-engine/animations/animation-events-not-cancelable.html
legacy-animation-engine/animations/unprefixed-keyframes.html
Comment 7 EWS Watchlist 2018-12-11 16:08:36 PST
Created attachment 357083 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 Antoine Quint 2018-12-12 05:54:53 PST
Created attachment 357117 [details]
Patch
Comment 9 Antoine Quint 2018-12-12 07:37:52 PST
Created attachment 357125 [details]
Patch
Comment 10 EWS Watchlist 2018-12-12 09:39:02 PST
Comment on attachment 357125 [details]
Patch

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

New failing tests:
legacy-animation-engine/imported/blink/animations/wrong-keyframe-name.html
legacy-animation-engine/animations/cross-fade-border-image-source.html
legacy-animation-engine/animations/simultaneous-start-left.html
legacy-animation-engine/animations/keyframes.html
legacy-animation-engine/animations/animation-direction.html
legacy-animation-engine/animations/unprefixed-keyframes.html
Comment 11 EWS Watchlist 2018-12-12 09:39:05 PST
Created attachment 357141 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 12 Dean Jackson 2018-12-12 11:50:55 PST
Comment on attachment 357125 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Frame used to expose an animation() method that returned a reference as it created its CSSAnimationController at creation time.

improve wording - "created its controller at creation time"

> Source/WebCore/ChangeLog:11
> +        We add a new Frame::existingAnimationController() method which returns a pointer and Frame::animationController() which creates
> +        a RefPtr<CSSAnimationController> lazily. We then replace the vast majority of calls to Frame::animation() to Frame::existingAnimationController().
> +        This matches the pattern used in the Web Animations codepath to access the DocumentTimeline.

What do we gain by doing this?

> Source/WebCore/page/FrameView.cpp:629
> +    if (!RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
> +        if (auto* animationController = frame().existingAnimationController())
> +            ASSERT(!animationController->hasAnimations());
> +    }

Put #if DEBUG around all this since you only end up with an ASSERT. The compiler probably removes it all anyway, but you might as well do it.
Comment 13 Antoine Quint 2018-12-13 04:36:05 PST
(In reply to Dean Jackson from comment #12)
> Comment on attachment 357125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357125&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Frame used to expose an animation() method that returned a reference as it created its CSSAnimationController at creation time.
> 
> improve wording - "created its controller at creation time"
> 
> > Source/WebCore/ChangeLog:11
> > +        We add a new Frame::existingAnimationController() method which returns a pointer and Frame::animationController() which creates
> > +        a RefPtr<CSSAnimationController> lazily. We then replace the vast majority of calls to Frame::animation() to Frame::existingAnimationController().
> > +        This matches the pattern used in the Web Animations codepath to access the DocumentTimeline.
> 
> What do we gain by doing this?

We no longer instantiate a CSSAnimationController if we are using the Web Animations codepath.

> > Source/WebCore/page/FrameView.cpp:629
> > +    if (!RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
> > +        if (auto* animationController = frame().existingAnimationController())
> > +            ASSERT(!animationController->hasAnimations());
> > +    }
> 
> Put #if DEBUG around all this since you only end up with an ASSERT. The
> compiler probably removes it all anyway, but you might as well do it.

Sure thing.
Comment 14 Antoine Quint 2020-03-09 13:03:14 PDT
This is mostly dead code now that the runtime flag for Web Animations is on by default everywhere. We'll nuke this in a few months. No need to improve that code.