[webkit-reviews] review granted: [Bug 183557] [Web Animations] Reflect timing properties and keyframe styles set by CSS for CSSAnimation and CSSTransition objects : [Attachment 335568] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 183557: [Web Animations] Reflect timing properties and keyframe styles set
by CSS for CSSAnimation and CSSTransition objects
https://bugs.webkit.org/show_bug.cgi?id=183557

Attachment 335568: Patch

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




--- Comment #2 from Dean Jackson <dino at apple.com> ---
Comment on attachment 335568
  --> https://bugs.webkit.org/attachment.cgi?id=335568
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?