Bug 179535

<rdar://problem/34932475>

This will be a primitive implementation, a spec-compliant implementation will follow in https://bugs.webkit.org/show_bug.cgi?id=179536.

Created attachment 326611 [details] Patch

Comment on attachment 326611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326611&action=review > Source/WebCore/animation/AnimationEffect.h:44 > + RefPtr<WebAnimation> animation() { return m_animation; } Returning a RefPtr causes ref churn. Return a raw pointer and make the function const. > Source/WebCore/animation/AnimationEffect.h:58 > + RefPtr<WebAnimation> m_animation; Can this be a Ref<>? > Source/WebCore/animation/AnimationTimeline.cpp:79 > + auto iterator = m_elementToAnimationsMap.find(&element); > + if (iterator != m_elementToAnimationsMap.end()) { > + iterator->value.append(&animation); > + return; > + } > + > + m_elementToAnimationsMap.set(&element, Vector<RefPtr<WebAnimation>> { &animation }); There's an ensure() on hash map that does both of these. What if animationWasAddedToElement() is called twice with the same animation and same element? > Source/WebCore/animation/AnimationTimeline.cpp:89 > + auto iterator = m_elementToAnimationsMap.find(&element); > + if (iterator == m_elementToAnimationsMap.end()) > + return; > + > + auto& animations = iterator->value; > + animations.removeFirst(&animation); This is two hash lookups. I think can can remove(iterator). > Source/WebCore/animation/AnimationTimeline.h:58 > + HashSet<RefPtr<WebAnimation>> animations() const { return m_animations; } > + Vector<RefPtr<WebAnimation>> animationsForElement(Element&); Lots of copying here. return references. > Source/WebCore/animation/AnimationTimeline.h:76 > + HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap; Should this really store owning references to Elements? You have to be careful to avoid creating a ref cycle between Elements and WebAnimations. > Source/WebCore/animation/WebAnimation.cpp:71 > + auto keyframeEffect = downcast<KeyframeEffect>(m_effect.get()); > + auto target = keyframeEffect->target(); Should these be auto& ? > Source/WebCore/animation/WebAnimation.cpp:86 > + auto keyframeEffect = downcast<KeyframeEffect>(effect.get()); > + auto target = keyframeEffect->target(); auto&? > Source/WebCore/animation/WebAnimation.cpp:111 > + auto keyframeEffect = downcast<KeyframeEffect>(m_effect.get()); > + auto target = keyframeEffect->target(); auto&? > Source/WebCore/dom/Document.cpp:7469 > + for (auto animation : m_timeline->animations()) auto&?

Created attachment 326624 [details] Patch

Comment on attachment 326624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326624&action=review > Source/WebCore/animation/AnimationEffect.h:45 > + void setAnimation(WebAnimation* animation) { m_animation = animation; } I think the current hotness here is to pass a RefPtr<WebAnimation>&& so the caller can WTFMove if they are passing ownership.

Committed r224705: <https://trac.webkit.org/changeset/224705>

This change introduced a crash seen with LayoutTest http/wpt/web-animations/timing-model/animations/updating-the-finished-state.html on WK1: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010e975899 WebCore::DocumentTimeline::currentTime() + 73 (DocumentTimeline.cpp:73) 1 com.apple.WebCore 0x000000010e975a4e WebCore::DocumentTimeline::updateAnimationSchedule() + 46 (DocumentTimeline.cpp:119) 2 com.apple.WebCore 0x000000010e975a0e WebCore::DocumentTimeline::performInvalidationTask() + 14 (DocumentTimeline.cpp:109) 3 com.apple.WebCore 0x000000010f04bf8a WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired() + 298 (GenericTaskQueue.cpp:65) 4 com.apple.WebCore 0x000000010f06b490 WebCore::ThreadTimers::sharedTimerFiredInternal() + 176 (ThreadTimers.cpp:121) 5 com.apple.WebCore 0x000000010f0a629f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 (MainThreadSharedTimerCF.cpp:75) 6 com.apple.CoreFoundation 0x00007fff93fedae4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 7 com.apple.CoreFoundation 0x00007fff93fed773 __CFRunLoopDoTimer + 1075 8 com.apple.CoreFoundation 0x00007fff93fed2ca __CFRunLoopDoTimers + 298 9 com.apple.CoreFoundation 0x00007fff93fe47c1 __CFRunLoopRun + 1841 10 com.apple.CoreFoundation 0x00007fff93fe3e28 CFRunLoopRunSpecific + 296 11 DumpRenderTree 0x000000010b49b1cd runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2568 (DumpRenderTree.mm:2027) 12 DumpRenderTree 0x000000010b49a571 dumpRenderTree(int, char const**) + 3097 (DumpRenderTree.mm:1291) 13 DumpRenderTree 0x000000010b49bce5 DumpRenderTreeMain(int, char const**) + 1627 (DumpRenderTree.mm:1407) 14 libdyld.dylib 0x00007fff9374c5ad start + 1 https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r224705%20(6189)/results.html

Reverted r224705 for reason: Introduced a LayoutTest crash on WK1. Committed r224709: <https://trac.webkit.org/changeset/224709>

Committed r224760: <https://trac.webkit.org/changeset/224760>