[webkit-reviews] review granted: [Bug 195929] RenderElement::startAnimation/animationFinished should take const Animation& : [Attachment 365109] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted zalan <zalan at apple.com>'s request
for review:
Bug 195929: RenderElement::startAnimation/animationFinished should take const
Animation&
https://bugs.webkit.org/show_bug.cgi?id=195929

Attachment 365109: Patch

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




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 365109
  --> https://bugs.webkit.org/attachment.cgi?id=365109
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.