Bug 195929 – RenderElement::startAnimation/animationFinished should take const Animation&

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

instead of const Animation*

[zalan]

Created attachment 365109 [details]
Patch

[zalan]

and this should be also applied all the way to the graphics layers.

[Daniel Bates]

Comment on attachment 365109 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1322
> +            if (!compositedRenderer->startAnimation(timeOffset, backingAnimationForCompositedRenderer().get(), m_blendingKeyframes)) {

Ok as-is. * instead of .get().

> Source/WebCore/animation/KeyframeEffect.cpp:1336
> +            compositedRenderer->animationFinished(m_blendingKeyframes.animationName(), backingAnimationForCompositedRenderer().get());

Ditto.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:295
> -        return renderer->startAnimation(timeOffset, m_animation.ptr(), m_keyframes);
> +        return renderer->startAnimation(timeOffset, m_animation.get(), m_keyframes);

Ditto.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:318
> -        renderer->animationFinished(m_keyframes.animationName());
> +        renderer->animationFinished(m_keyframes.animationName(), m_animation.get());

Ok as-is, but why? Animation is unused. Symmetry?😀  Seems unnecessary in my opinion.🤪

Also ok-as is with .get, could use *.

> Source/WebCore/rendering/RenderElement.h:229
> -    virtual bool startAnimation(double /* timeOffset */, const Animation*, const KeyframeList&) { return false; }
> +    virtual bool startAnimation(double /* timeOffset */, const Animation&, const KeyframeList&) { return false; }

Nice.

> Source/WebCore/rendering/RenderElement.h:232
> -    virtual void animationFinished(const String& /* name */) { }
> +    virtual void animationFinished(const String& /* name */, const Animation&) { }

Ok as-is. 😐

> Source/WebCore/rendering/RenderLayerBacking.cpp:2793
> -bool RenderLayerBacking::startAnimation(double timeOffset, const Animation* anim, const KeyframeList& keyframes)
> +bool RenderLayerBacking::startAnimation(double timeOffset, const Animation& animation, const KeyframeList& keyframes)

Nice.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2849
> -    if (hasOpacity && m_graphicsLayer->addAnimation(opacityVector, IntSize(), anim, keyframes.animationName(), timeOffset))
> +    if (hasOpacity && m_graphicsLayer->addAnimation(opacityVector, IntSize(), &animation, keyframes.animationName(), timeOffset))

Ok as-is. Could modernize with IntSize { }.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2852
> -    if (hasFilter && m_graphicsLayer->addAnimation(filterVector, IntSize(), anim, keyframes.animationName(), timeOffset))
> +    if (hasFilter && m_graphicsLayer->addAnimation(filterVector, IntSize(), &animation, keyframes.animationName(), timeOffset))

Ditto.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2856
> -    if (hasBackdropFilter && m_graphicsLayer->addAnimation(backdropFilterVector, IntSize(), anim, keyframes.animationName(), timeOffset))
> +    if (hasBackdropFilter && m_graphicsLayer->addAnimation(backdropFilterVector, IntSize(), &animation, keyframes.animationName(), timeOffset))

Ditto.

> Source/WebCore/rendering/RenderLayerBacking.h:197
> -    bool startAnimation(double timeOffset, const Animation* anim, const KeyframeList& keyframes);
> +    bool startAnimation(double timeOffset, const Animation&, const KeyframeList& keyframes);

Ok as-is. No need for last parameter's name.

> Source/WebCore/rendering/RenderLayerModelObject.h:76
> +    bool startAnimation(double timeOffset, const Animation&, const KeyframeList& keyframes) override;

Ok as-is. No need for last parameter's name.

[zalan]

Created attachment 365169 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365169 [details]
Patch

Clearing flags on attachment: 365169

Committed r243151: <https://trac.webkit.org/changeset/243151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49025813>