Bug 202192 – [Web Animations] Move Document.getAnimations() to DocumentOrShadowRoot

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

This method moved to also apply to shadow roots, see https://drafts.csswg.org/web-animations/#dom-documentorshadowroot-getanimations.

[Radar WebKit Bug Importer]

<rdar://problem/55697775>

[Antoine Quint]

Created attachment 395563 [details]
Patch

[Antti Koivisto]

Comment on attachment 395563 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8104
> -Vector<RefPtr<WebAnimation>> Document::getAnimations()
> +Vector<RefPtr<WebAnimation>> Document::getAnimations(IncludeAnimationsTargetingElementsInShadowRoot includeAnimationsTargetingElementsInShadowRoot)

It might be cleaner to have a separate internal function that takes the argument and have the argument-free getAnimations() just call that.

Internal clients would all switch to the new function.

> Source/WebCore/dom/Document.cpp:8120
> +    for (auto& animation : m_timeline->getAnimations()) {
> +        auto* effect = animation->effect();
> +        ASSERT(is<KeyframeEffect>(animation->effect()));
> +        auto* target = downcast<KeyframeEffect>(*effect).target();

Couldn't getAnimations()/new internal function just return a list of KeyframeEffects is those are the only things it can contain?

> Source/WebCore/dom/Document.h:1488
> +    Vector<RefPtr<WebAnimation>> getAnimations(IncludeAnimationsTargetingElementsInShadowRoot = IncludeAnimationsTargetingElementsInShadowRoot::No);

Then you wouldn't need a default parameter either.

[Antti Koivisto]

Comment on attachment 395563 [details]
Patch

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

> Source/WebCore/dom/ShadowRoot.cpp:384
> +Vector<RefPtr<WebAnimation>> ShadowRoot::getAnimations()
> +{
> +    Vector<RefPtr<WebAnimation>> animations;
> +    for (auto& animation : document().getAnimations(Document::IncludeAnimationsTargetingElementsInShadowRoot::Yes)) {
> +        auto* effect = animation->effect();
> +        ASSERT(is<KeyframeEffect>(animation->effect()));
> +        auto* target = downcast<KeyframeEffect>(*effect).target();
> +        ASSERT(target);
> +        if (target->containingShadowRoot() == this)
> +            animations.append(animation);
> +    }
> +    return animations;
> +}

You can put code shared between Document and ShadowRoot to TreeScope.

[Antoine Quint]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 395563 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395563&action=review
> 
> > Source/WebCore/dom/Document.cpp:8104
> > -Vector<RefPtr<WebAnimation>> Document::getAnimations()
> > +Vector<RefPtr<WebAnimation>> Document::getAnimations(IncludeAnimationsTargetingElementsInShadowRoot includeAnimationsTargetingElementsInShadowRoot)
> 
> It might be cleaner to have a separate internal function that takes the
> argument and have the argument-free getAnimations() just call that.
> 
> Internal clients would all switch to the new function.
> 
> > Source/WebCore/dom/Document.cpp:8120
> > +    for (auto& animation : m_timeline->getAnimations()) {
> > +        auto* effect = animation->effect();
> > +        ASSERT(is<KeyframeEffect>(animation->effect()));
> > +        auto* target = downcast<KeyframeEffect>(*effect).target();
> 
> Couldn't getAnimations()/new internal function just return a list of
> KeyframeEffects is those are the only things it can contain?
> 
> > Source/WebCore/dom/Document.h:1488
> > +    Vector<RefPtr<WebAnimation>> getAnimations(IncludeAnimationsTargetingElementsInShadowRoot = IncludeAnimationsTargetingElementsInShadowRoot::No);
> 
> Then you wouldn't need a default parameter either.

All of these are useful comments! I think Document::getAnimations() will implement the API and call a new internal method Document::filterAnimations() which will take an optional lambda that will run for each Element targeted by an animation to filter it out.

[Antoine Quint]

Created attachment 395573 [details]
Patch

[Antti Koivisto]

Comment on attachment 395573 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8111
> +Vector<RefPtr<WebAnimation>> Document::getMatchingAnimations(const WTF::Function<bool(Element&)>& function)

We don't usually name functions getSomething. API functions like getAnimations() are the exception. Maybe just matchingAnimations()?

> Source/WebCore/dom/Document.cpp:8129
> +    for (auto& animation : m_timeline->getAnimations()) {
> +        auto* effect = animation->effect();
> +        ASSERT(is<KeyframeEffect>(animation->effect()));
> +        auto* target = downcast<KeyframeEffect>(*effect).target();
> +        ASSERT(target);
> +        if (function(*target))
> +            animations.append(animation);
> +    }
> +    return animations;

This function doesn't really have much to do with Document. Maybe it could live in DocumentTimeline instead?

[Antoine Quint]

(In reply to Antti Koivisto from comment #7)
> Comment on attachment 395573 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395573&action=review
> 
> > Source/WebCore/dom/Document.cpp:8111
> > +Vector<RefPtr<WebAnimation>> Document::getMatchingAnimations(const WTF::Function<bool(Element&)>& function)
> 
> We don't usually name functions getSomething. API functions like
> getAnimations() are the exception. Maybe just matchingAnimations()?

Good point. I'll change the method name.

> > Source/WebCore/dom/Document.cpp:8129
> > +    for (auto& animation : m_timeline->getAnimations()) {
> > +        auto* effect = animation->effect();
> > +        ASSERT(is<KeyframeEffect>(animation->effect()));
> > +        auto* target = downcast<KeyframeEffect>(*effect).target();
> > +        ASSERT(target);
> > +        if (function(*target))
> > +            animations.append(animation);
> > +    }
> > +    return animations;
> 
> This function doesn't really have much to do with Document. Maybe it could
> live in DocumentTimeline instead?

While we don't support that well yet, eventually we should return animations for all timelines associated with the provided document, so it belongs on Document.

[Antoine Quint]

Committed r259577: <https://trac.webkit.org/changeset/259577>