Before we can ship Web Animations, we need to implement https://drafts.csswg.org/web-animations/#replacing-animations.
<rdar://problem/55697719>
Created attachment 380002 [details] Patch
Comment on attachment 380002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380002&action=review > Source/WebCore/animation/AnimationTimeline.cpp:195 > + Vector<RefPtr<WebAnimation>> sortedWebAnimations; Initialize the vector with the size of webAnimations.size() > Source/WebCore/animation/DocumentTimeline.cpp:450 > + auto animations = animationsForElement(*target, AnimationTimeline::Ordering::Sorted); > + for (auto& animationWithHigherCompositeOrder : WTF::makeReversedRange(animations)) { If you always access them in this reversed manner, why not have Ordering::SortedDescending or something like that, to avoid sorting then reversing? > Source/WebCore/animation/DocumentTimeline.cpp:476 > + for (const auto& animation : m_allAnimations) { Do you need the const here? > Source/WebCore/animation/DocumentTimeline.cpp:480 > + animation->setReplaceState(WebAnimation::ReplaceState::Removed); Also, this looks like a non-const method? > Source/WebCore/animation/DocumentTimeline.cpp:488 > + // 6. If animation has a document for timing, then append removeEvent to its document for timing's pending animation > + // event queue along with its target, animation. For the scheduled event time, use the result of applying the procedure > + // to convert timeline time to origin-relative time to the current time of the timeline with which animation is associated. > + // Otherwise, queue a task to dispatch removeEvent at animation. The task source for this task is the DOM manipulation task source. It's not clear you're doing this. What does queue a task to dispatch removeEvent at animation mean?
(In reply to Dean Jackson from comment #3) > Comment on attachment 380002 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380002&action=review > > > Source/WebCore/animation/AnimationTimeline.cpp:195 > > + Vector<RefPtr<WebAnimation>> sortedWebAnimations; > > Initialize the vector with the size of webAnimations.size() Will do. > > Source/WebCore/animation/DocumentTimeline.cpp:450 > > + auto animations = animationsForElement(*target, AnimationTimeline::Ordering::Sorted); > > + for (auto& animationWithHigherCompositeOrder : WTF::makeReversedRange(animations)) { > > If you always access them in this reversed manner, why not have > Ordering::SortedDescending or something like that, to avoid sorting then > reversing? It would make animationsForElement() more complex while I don't believe there's any cost in using this reversed iterator. But I could be mistaken! > > Source/WebCore/animation/DocumentTimeline.cpp:476 > > + for (const auto& animation : m_allAnimations) { > > Do you need the const here? > > > Source/WebCore/animation/DocumentTimeline.cpp:480 > > + animation->setReplaceState(WebAnimation::ReplaceState::Removed); > > Also, this looks like a non-const method? Correct, I'll remove the const here. > > Source/WebCore/animation/DocumentTimeline.cpp:488 > > + // 6. If animation has a document for timing, then append removeEvent to its document for timing's pending animation > > + // event queue along with its target, animation. For the scheduled event time, use the result of applying the procedure > > + // to convert timeline time to origin-relative time to the current time of the timeline with which animation is associated. > > + // Otherwise, queue a task to dispatch removeEvent at animation. The task source for this task is the DOM manipulation task source. > > It's not clear you're doing this. What does queue a task to dispatch > removeEvent at animation mean? It's done by WebAnimation::enqueueAnimationPlaybackEvent().
Committed r250603: <https://trac.webkit.org/changeset/250603>
It looks like the changes in https://trac.webkit.org/changeset/250603/webkit is causing testing to exit early with 50 crashes. Build: https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/7061 results: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r250603%20(7061)/results.html I was able to reproduce this locally by running the effected tests on 250603 which failed, and 250602 which passed. Can this be addressed quickly? Otherwise we will need to roll this back.
Reverted r250603 for reason: Caused testing to exit early with crashes on Mac Committed r250607: <https://trac.webkit.org/changeset/250607>
Committed r250617: <https://trac.webkit.org/changeset/250617>
It looks like the changes in https://trac.webkit.org/changeset/250617/webkit caused this test for crash on debug: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fupdate-and-send-events-replacement.html Crash Log: No crash log found for DumpRenderTree:41843. stdout: stderr: ASSERTION FAILED: !animation.timeline() || animation.timeline() == this ./animation/AnimationTimeline.cpp(75) : virtual void WebCore::AnimationTimeline::removeAnimation(WebCore::WebAnimation &) 1 0x10ce41cb9 WTFCrash 2 0x121eb0a0b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x1240b53d7 WebCore::AnimationTimeline::removeAnimation(WebCore::WebAnimation&) 4 0x1240be557 WebCore::DocumentTimeline::removeAnimation(WebCore::WebAnimation&) 5 0x1240bf088 WebCore::DocumentTimeline::removeReplacedAnimations() 6 0x1240be89a WebCore::DocumentTimeline::internalUpdateAnimationsAndSendEvents() 7 0x1240be60c WebCore::DocumentTimeline::updateAnimationsAndSendEvents(double) 8 0x1247214c6 WebCore::Document::updateAnimationsAndSendEvents(double) 9 0x12545f8d1 WebCore::Page::updateRendering() 10 0x1163b6966 -[WebView(WebPrivate) _viewWillDrawInternal] 11 0x1163de04c LayerFlushController::flushLayers() 12 0x1164d0d39 WebViewLayerFlushScheduler::layerFlushCallback() 13 0x1164d22c8 WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0::operator()() const 14 0x1164d2289 WTF::Detail::CallableWrapper<WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0, void>::call() 15 0x121ebe15a WTF::Function<void ()>::operator()() const 16 0x1256bbffc WebCore::RunLoopObserver::runLoopObserverFired() 17 0x1256bbf60 WebCore::RunLoopObserver::runLoopObserverFired(__CFRunLoopObserver*, unsigned long, void*) 18 0x7fff34ed2f28 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ 19 0x7fff34ed2e5d __CFRunLoopDoObservers 20 0x7fff34e756a7 __CFRunLoopRun 21 0x7fff34e74ebe CFRunLoopRunSpecific 22 0x10c18036b runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) 23 0x10c17f56a runTestingServerLoop() 24 0x10c17ec93 dumpRenderTree(int, char const**) 25 0x10c180d7d DumpRenderTreeMain(int, char const**) 26 0x10c209202 main 27 0x7fff60dcb3d5 start
The crash is specific to Debug builds and affects both WK1 and WK2.
Two of the subtests cause this crash: "Removes an animation after updating another animation's timeline" and "Removes an animation after updating its timeline".
This (In reply to Truitt Savell from comment #9) > It looks like the changes in https://trac.webkit.org/changeset/250617/webkit caused this test for crash on debug: > imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html This is slowing down macOS Debug WK1 EWS, can we either fix this soon, revert the commit or mark the test as failing.
The crash fix is covered by bug 202583 which has a patch up for review.