Bug 183557 – [Web Animations] Reflect timing properties and keyframe styles set by CSS for CSSAnimation and CSSTransition objects

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Animations

[Antoine Quint]

[Web Animations] Reflect timing properties and keyframe styles set by CSS for CSSAnimation and CSSTransition objects

[Antoine Quint]

Created attachment 335568 [details]
Patch

[Dean Jackson]

Comment on attachment 335568 [details]
Patch

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

> Source/WebCore/animation/AnimationEffectTimingReadOnly.cpp:232
> +void AnimationEffectTimingReadOnly::setTimingFunction(const RefPtr<TimingFunction> timingFunction)

I think this can be a RefPtr<TimingFunction>& to avoid a cycle.

> Source/WebCore/animation/CSSAnimation.cpp:59
> +

Get a local variable to avoid calling backingAnimation so many times:

const Animation& animation = backingAnimation();

> Source/WebCore/animation/CSSAnimation.cpp:95
> +    auto* timing = effect()->timing();
> +    timing->setFill(fillMode);
> +    timing->setDirection(direction);
> +    timing->setIterations(backingAnimation().iterationCount());

Why not just call setFill() etc from inside the switch statements and avoid local variables?

> Source/WebCore/animation/DeclarativeAnimation.cpp:38
> +DeclarativeAnimation::DeclarativeAnimation(Document& document, const Animation& backingAnimation)
> +    : WebAnimation(document)
> +    , m_backingAnimation(const_cast<Animation&>(backingAnimation))

You should make m_backingAnimation a const Animation&, that way you won't need to const_cast. It shouldn't create a Ref<>

> Source/WebCore/animation/DeclarativeAnimation.cpp:44
> +void DeclarativeAnimation::setBackingAnimation(const Animation& backingAnimation)
> +{
> +    m_backingAnimation = const_cast<Animation&>(backingAnimation);

When do you set this outside the constructor? I don't think you need this method (nor should you have it).

> Source/WebCore/animation/DeclarativeAnimation.cpp:56
> +    auto effect = KeyframeEffectReadOnly::create(target);
> +    setEffect(WTFMove(effect));

No need for the variable.

> Source/WebCore/animation/DeclarativeAnimation.h:43
> +    void setBackingAnimation(const Animation&);

See above.

> Source/WebCore/animation/DeclarativeAnimation.h:48
> +    virtual void initialize(Element&);

Can't this be a const Element&? You copy the timeline out in a subclass, but i think that's const too.

> Source/WebCore/animation/DeclarativeAnimation.h:52
> +    Ref<Animation> m_backingAnimation;

See above.

> Source/WebCore/bindings/js/JSWebAnimationCustom.cpp:29
> +#include "CSSAnimation.h"

Why?

[Antoine Quint]

(In reply to Dean Jackson from comment #2)
> Comment on attachment 335568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335568&action=review
> 
> > Source/WebCore/animation/AnimationEffectTimingReadOnly.cpp:232
> > +void AnimationEffectTimingReadOnly::setTimingFunction(const RefPtr<TimingFunction> timingFunction)
> 
> I think this can be a RefPtr<TimingFunction>& to avoid a cycle.

Will fix.

> > Source/WebCore/animation/CSSAnimation.cpp:59
> > +
> 
> Get a local variable to avoid calling backingAnimation so many times:
> 
> const Animation& animation = backingAnimation();

Will add `auto& animation = backingAnimation();`

> > Source/WebCore/animation/CSSAnimation.cpp:95
> > +    auto* timing = effect()->timing();
> > +    timing->setFill(fillMode);
> > +    timing->setDirection(direction);
> > +    timing->setIterations(backingAnimation().iterationCount());
> 
> Why not just call setFill() etc from inside the switch statements and avoid
> local variables?

Good point, will fix.

> > Source/WebCore/animation/DeclarativeAnimation.cpp:38
> > +DeclarativeAnimation::DeclarativeAnimation(Document& document, const Animation& backingAnimation)
> > +    : WebAnimation(document)
> > +    , m_backingAnimation(const_cast<Animation&>(backingAnimation))
> 
> You should make m_backingAnimation a const Animation&, that way you won't
> need to const_cast. It shouldn't create a Ref<>

This is using the same approach taken by AnimationBase. Note that Animation.h has the following:

Animation& operator=(const Animation& o);

> > Source/WebCore/animation/DeclarativeAnimation.cpp:44
> > +void DeclarativeAnimation::setBackingAnimation(const Animation& backingAnimation)
> > +{
> > +    m_backingAnimation = const_cast<Animation&>(backingAnimation);
> 
> When do you set this outside the constructor? I don't think you need this
> method (nor should you have it).

We need to update the backing animation when updating CSSAnimation objects during style resolution, see the patch in webkit.org/b/183558.

> > Source/WebCore/animation/DeclarativeAnimation.cpp:56
> > +    auto effect = KeyframeEffectReadOnly::create(target);
> > +    setEffect(WTFMove(effect));
> 
> No need for the variable.

Will fix.

> > Source/WebCore/animation/DeclarativeAnimation.h:43
> > +    void setBackingAnimation(const Animation&);
> 
> See above.
> 
> > Source/WebCore/animation/DeclarativeAnimation.h:48
> > +    virtual void initialize(Element&);
> 
> Can't this be a const Element&? You copy the timeline out in a subclass, but
> i think that's const too.

It can, although eventually it'll need to be const_cast'd for final call to the KeyframeEffectReadOnly(ClassType, Ref<AnimationEffectTimingReadOnly>&&, Element*) constructor.

Will fix.

> > Source/WebCore/animation/DeclarativeAnimation.h:52
> > +    Ref<Animation> m_backingAnimation;
> 
> See above.
> 
> > Source/WebCore/bindings/js/JSWebAnimationCustom.cpp:29
> > +#include "CSSAnimation.h"
> 
> Why?

Must have been needed some time during development of this patch, but it turns out it's not needed, thanks for catching this.

[Antoine Quint]

*** This bug has been marked as a duplicate of bug 183504 ***