Bug 202192 - [Web Animations] Move Document.getAnimations() to DocumentOrShadowRoot
Summary: [Web Animations] Move Document.getAnimations() to DocumentOrShadowRoot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-25 02:56 PDT by Antoine Quint
Modified: 2020-04-06 10:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (17.60 KB, patch)
2020-04-06 07:25 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (17.29 KB, patch)
2020-04-06 09:43 PDT, Antoine Quint
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2019-09-25 02:56:19 PDT
This method moved to also apply to shadow roots, see https://drafts.csswg.org/web-animations/#dom-documentorshadowroot-getanimations.
Comment 1 Radar WebKit Bug Importer 2019-09-25 02:56:44 PDT
<rdar://problem/55697775>
Comment 2 Antoine Quint 2020-04-06 07:25:34 PDT
Created attachment 395563 [details]
Patch
Comment 3 Antti Koivisto 2020-04-06 07:42:18 PDT
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.
Comment 4 Antti Koivisto 2020-04-06 07:44:06 PDT
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.
Comment 5 Antoine Quint 2020-04-06 08:38:14 PDT
(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.
Comment 6 Antoine Quint 2020-04-06 09:43:08 PDT
Created attachment 395573 [details]
Patch
Comment 7 Antti Koivisto 2020-04-06 09:55:42 PDT
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?
Comment 8 Antoine Quint 2020-04-06 10:08:24 PDT
(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.
Comment 9 Antoine Quint 2020-04-06 10:41:53 PDT
Committed r259577: <https://trac.webkit.org/changeset/259577>