Bug 179535 – [Web Animations] Implement getAnimations()

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

[Antoine Quint]

The Web Animations spec defines a getAnimations() method that is exposed on both Document and, through the Animatable interface, on Element.

[Antoine Quint]

<rdar://problem/34932475>

[Antoine Quint]

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

[Antoine Quint]

Created attachment 326611 [details]
Patch

[Simon Fraser (smfr)]

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

[Antoine Quint]

Created attachment 326624 [details]
Patch

[Simon Fraser (smfr)]

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.

[Antoine Quint]

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

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r224705 for reason:

Introduced a LayoutTest crash on WK1.

Committed r224709: <https://trac.webkit.org/changeset/224709>

[Antoine Quint]

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