[webkit-reviews] review granted: [Bug 191153] [Web Animations] Make document.getAnimations() return declarative animations in the correct order : [Attachment 353607] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 191153: [Web Animations] Make document.getAnimations() return declarative
animations in the correct order
https://bugs.webkit.org/show_bug.cgi?id=191153

Attachment 353607: Patch

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




--- Comment #4 from Dean Jackson <dino at apple.com> ---
Comment on attachment 353607
  --> https://bugs.webkit.org/attachment.cgi?id=353607
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