Bug 191153 – [Web Animations] Make document.getAnimations() return declarative animations in the correct order

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Web Animations] Make document.getAnimations() return declarative animations in the correct order

[Antoine Quint]

Created attachment 353607 [details]
Patch

[EWS Watchlist]

Comment on attachment 353607 [details]
Patch

Attachment 353607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9815304

New failing tests:
fast/layers/no-clipping-overflow-hidden-added-after-transform.html

[EWS Watchlist]

Created attachment 353620 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Dean Jackson]

Comment on attachment 353607 [details]
Patch

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

> Source/WebCore/animation/AnimationTimeline.cpp:88
> +    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>* map = &m_elementToAnimationsMap;

auto*

> Source/WebCore/animation/AnimationTimeline.cpp:92
> +    if (is<CSSTransition>(animation) && downcast<CSSTransition>(animation).owningElement())
> +        map = &m_elementToCSSTransitionsMap;
> +    else if (is<CSSAnimation>(animation) && downcast<CSSAnimation>(animation).owningElement())
> +        map = &m_elementToCSSAnimationsMap;

I suggest doing this:

auto* map  = [&] () -> HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>* {
    if (is<CSSTransition>(animation) && downcast<CSSTransition>(animation).owningElement())
        return map = &m_elementToCSSTransitionsMap;
    else if (is<CSSAnimation>(animation) && downcast<CSSAnimation>(animation).owningElement())
        return &m_elementToCSSAnimationsMap;
    return &m_elementToAnimationsMap;
}();

Then in fact you could probably do it with just an auto& and return the objects directly.

Also, you might want to typedef (using) that HashMap type so it's easier to read.

> Source/WebCore/animation/AnimationTimeline.cpp:134
> +    // First, we clear this animation from one of the m_elementToCSSAnimationsMap, m_elementToCSSTransitionsMap,
> +    // m_elementToAnimationsMap or m_elementToCompletedCSSTransitionByCSSPropertyID map, whichever is relevant to
> +    // this type of animation.

This comment doesn't add anything.

> Source/WebCore/animation/AnimationTimeline.cpp:338
> +        return HashMap<CSSPropertyID, RefPtr<CSSTransition>> { };

You should using/typedef this one as well.

> Source/WebCore/animation/AnimationTimeline.h:82
> +    HashMap<Element*, HashMap<CSSPropertyID, RefPtr<CSSTransition>>> m_elementToCompletedCSSTransitionByCSSPropertyID;

Use using here.

> Source/WebCore/animation/DocumentTimeline.cpp:161
> +        // Otherwise, if A and B have different transition generation values, sort by their corresponding transition generation in ascending order.

generation time

[Antoine Quint]

Committed r237726: <https://trac.webkit.org/changeset/237726>

[Radar WebKit Bug Importer]

<rdar://problem/45756154>