Created attachment 357074 [details] Patch

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

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

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

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

Created attachment 357117 [details] Patch

Created attachment 357125 [details] Patch

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

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

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

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.