Bug 195929

Summary: RenderElement::startAnimation/animationFinished should take const Animation&
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dbates, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description zalan 2019-03-18 19:17:19 PDT
instead of const Animation*
Comment 1 zalan 2019-03-18 19:19:13 PDT
Created attachment 365109 [details]
Patch
Comment 2 zalan 2019-03-18 19:19:43 PDT
and this should be also applied all the way to the graphics layers.
Comment 3 Daniel Bates 2019-03-18 20:43:54 PDT
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.
Comment 4 zalan 2019-03-19 09:20:49 PDT
Created attachment 365169 [details]
Patch
Comment 5 WebKit Commit Bot 2019-03-19 10:59:32 PDT
Comment on attachment 365169 [details]
Patch

Clearing flags on attachment: 365169

Committed r243151: <https://trac.webkit.org/changeset/243151>
Comment 6 WebKit Commit Bot 2019-03-19 10:59:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-03-19 11:00:24 PDT
<rdar://problem/49025813>