[Web Animations] Reflect timing properties and keyframe styles set by CSS for CSSAnimation and CSSTransition objects
Created attachment 335568 [details] Patch
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?
(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.
*** This bug has been marked as a duplicate of bug 183504 ***