[webkit-reviews] review denied: [Bug 179535] [Web Animations] Implement getAnimations() : [Attachment 326611] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 179535: [Web Animations] Implement getAnimations()
https://bugs.webkit.org/show_bug.cgi?id=179535

Attachment 326611: Patch

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




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 326611
  --> https://bugs.webkit.org/attachment.cgi?id=326611
Patch

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

> Source/WebCore/animation/AnimationEffect.h:44
> +    RefPtr<WebAnimation> animation() { return m_animation; }

Returning a RefPtr causes ref churn. Return a raw pointer and make the function
const.

> Source/WebCore/animation/AnimationEffect.h:58
> +    RefPtr<WebAnimation> m_animation;

Can this be a Ref<>?

> Source/WebCore/animation/AnimationTimeline.cpp:79
> +    auto iterator = m_elementToAnimationsMap.find(&element);
> +    if (iterator != m_elementToAnimationsMap.end()) {
> +	   iterator->value.append(&animation);
> +	   return;
> +    }
> +
> +    m_elementToAnimationsMap.set(&element, Vector<RefPtr<WebAnimation>> {
&animation });

There's an ensure() on hash map that does both of these.

What if animationWasAddedToElement() is called twice with the same animation
and same element?

> Source/WebCore/animation/AnimationTimeline.cpp:89
> +    auto iterator = m_elementToAnimationsMap.find(&element);
> +    if (iterator == m_elementToAnimationsMap.end())
> +	   return;
> +
> +    auto& animations = iterator->value;
> +    animations.removeFirst(&animation);

This is two hash lookups. I think can can remove(iterator).

> Source/WebCore/animation/AnimationTimeline.h:58
> +    HashSet<RefPtr<WebAnimation>> animations() const { return m_animations;
}
> +    Vector<RefPtr<WebAnimation>> animationsForElement(Element&);

Lots of copying here. return references.

> Source/WebCore/animation/AnimationTimeline.h:76
> +    HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>>
m_elementToAnimationsMap;

Should this really store owning references to Elements? You have to be careful
to avoid creating a ref cycle between Elements and WebAnimations.

> Source/WebCore/animation/WebAnimation.cpp:71
> +	       auto keyframeEffect = downcast<KeyframeEffect>(m_effect.get());
> +	       auto target = keyframeEffect->target();

Should these be auto& ?

> Source/WebCore/animation/WebAnimation.cpp:86
> +	       auto keyframeEffect = downcast<KeyframeEffect>(effect.get());
> +	       auto target = keyframeEffect->target();

auto&?

> Source/WebCore/animation/WebAnimation.cpp:111
> +	   auto keyframeEffect = downcast<KeyframeEffect>(m_effect.get());
> +	   auto target = keyframeEffect->target();

auto&?

> Source/WebCore/dom/Document.cpp:7469
> +	   for (auto animation : m_timeline->animations())

auto&?