[webkit-reviews] review granted: [Bug 202192] [Web Animations] Move Document.getAnimations() to DocumentOrShadowRoot : [Attachment 395563] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 202192: [Web Animations] Move Document.getAnimations() to
DocumentOrShadowRoot
https://bugs.webkit.org/show_bug.cgi?id=202192

Attachment 395563: Patch

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 395563
  --> https://bugs.webkit.org/attachment.cgi?id=395563
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.