[Web Animations] Create, update and remove CSSAnimation and CSSTransition objects on the DocumentTimeline
Created attachment 335572 [details] Patch
Comment on attachment 335572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335572&action=review > Source/WebCore/animation/AnimationTimeline.cpp:97 > return Vector<RefPtr<WebAnimation>>(); Vector<RefPtr<WebAnimation>> { } > Source/WebCore/animation/AnimationTimeline.cpp:148 > + return HashMap<String, RefPtr<CSSAnimation>>(); use { } syntax > Source/WebCore/animation/AnimationTimeline.cpp:159 > + // We've found the name of this animation in our list of previous animations, this means we've already > + // created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current > + // animation object for this animation name. > + if (namesOfPreviousAnimations.contains(name)) > + cssAnimationsByName.get(name)->setBackingAnimation(currentAnimation); I think you should change AnimationList to export Ref<Animation>. Also I don't think setBackingAnimation needs to take a const, which would avoid the const_cast in the previous patch. > Source/WebCore/animation/AnimationTimeline.cpp:161 > + // Otherwise we are dealing with a new animation name and must create a CSSAnimation for it. > + else if (currentAnimation.isValidAnimation()) We usually put the comment inside the if, even if it means using { } > Source/WebCore/animation/AnimationTimeline.cpp:208 > + return HashMap<CSSPropertyID, RefPtr<CSSTransition>>(); { } (I even wonder if you need to define the type, since it is inferred.) > Source/WebCore/animation/AnimationTimeline.h:89 > + HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap; > + HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap; > + HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap; I'm not sure if my advice to use Element* was good, but we'll see :) > Source/WebCore/animation/DocumentTimeline.cpp:166 > + if (m_document && (!elementToAnimationsMap().isEmpty() || !elementToCSSAnimationsMap().isEmpty() || !elementToCSSTransitionsMap().isEmpty())) { Make a hasAnimations() helper on AnimationTimeline > Source/WebCore/style/StyleTreeResolver.cpp:333 > + shouldRecompositeLayer = element.styleResolutionShouldRecompositeLayer() || shouldRecompositeLayer; use |= ? > LayoutTests/webanimations/css-animations.html:26 > + test(() => { I like _ instead of () > LayoutTests/webanimations/css-animations.html:44 > + const animation = target.getAnimations()[0]; So does getAnimations force a style recalc? > LayoutTests/webanimations/css-animations.html:46 > + assert_true(animation instanceof CSSAnimation, "The animation is a CSSAnimation."); > + assert_true(animation instanceof Animation, "The animation is an Animation."); Should be the other way around > LayoutTests/webanimations/css-animations.html:66 > + assert_equals(computedTiming.currentIteration, 1, "The animations's computed timing currentIteration property matches the properties set by CSS"); I assume this means the 2nd iteration? > LayoutTests/webanimations/css-animations.html:71 > + assert_equals(computedTiming.iterationStart, 0, "The animations's computed timing iterationStart property matches the properties set by CSS"); Not sure what this means > LayoutTests/webanimations/css-animations.html:74 > + assert_equals(computedTiming.progress, 0.5, "The animations's computed timing progress property matches the properties set by CSS"); Huh? Is this progress within the iteration? > LayoutTests/webanimations/css-animations.html:81 > + assert_equals(keyframes[0].offset, 0, "The animation's effect's first keyframe has a 0 offset."); Lucky we don't have a CSS property called offset. > LayoutTests/webanimations/css-animations.html:98 > +targetTest(target => { > + target.style.animationDelay = "-1s"; > + target.style.animationDuration = "2s"; > + target.style.animationName = "animation"; > + > + assert_equals(target.getAnimations()[0].effect.timing.delay, -1000, "The animation's delay matches the initial animation-delay property."); > + > + target.style.animationDelay = 0; > + assert_equals(target.getAnimations()[0].effect.timing.delay, 0, "The animation's delay matches the updated animation-delay property."); > +}, "Web Animations should reflect the animation-delay property."); Didn't you just test this? > LayoutTests/webanimations/css-animations.html:144 > + assert_equals(target.getAnimations()[0].effect.timing.iterations, 1, "The animation's duration matches the updated animation-iteration-count property."); Add one like this too: 189 assert_equals(target.getAnimations()[0], initialAnimation, "The animation object remained the same instance throughout the test."); > LayoutTests/webanimations/css-animations.html:172 > + assert_not_equals(updatedAnimation, initialAnimation, "Changing the animation-name property generates a different CSSAnimation object."); You should test this right after getting updatedAnimation
(In reply to Dean Jackson from comment #2) > Comment on attachment 335572 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335572&action=review > > > Source/WebCore/animation/AnimationTimeline.cpp:97 > > return Vector<RefPtr<WebAnimation>>(); > > Vector<RefPtr<WebAnimation>> { } Will fix. > > Source/WebCore/animation/AnimationTimeline.cpp:148 > > + return HashMap<String, RefPtr<CSSAnimation>>(); > > use { } syntax Will fix. > > Source/WebCore/animation/AnimationTimeline.cpp:159 > > + // We've found the name of this animation in our list of previous animations, this means we've already > > + // created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current > > + // animation object for this animation name. > > + if (namesOfPreviousAnimations.contains(name)) > > + cssAnimationsByName.get(name)->setBackingAnimation(currentAnimation); > > I think you should change AnimationList to export Ref<Animation>. Also I > don't think setBackingAnimation needs to take a const, which would avoid the > const_cast in the previous patch. Right. I'd like to do this as a separate task. AnimationList is most likely due for a good amount of updates. > > Source/WebCore/animation/AnimationTimeline.cpp:161 > > + // Otherwise we are dealing with a new animation name and must create a CSSAnimation for it. > > + else if (currentAnimation.isValidAnimation()) > > We usually put the comment inside the if, even if it means using { } Will fix. > > Source/WebCore/animation/AnimationTimeline.cpp:208 > > + return HashMap<CSSPropertyID, RefPtr<CSSTransition>>(); > > { } > > (I even wonder if you need to define the type, since it is inferred.) I tried, but we do (error: cannot deduce lambda return type from initializer list). > > Source/WebCore/animation/AnimationTimeline.h:89 > > + HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap; > > + HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap; > > + HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap; > > I'm not sure if my advice to use Element* was good, but we'll see :) :) > > Source/WebCore/animation/DocumentTimeline.cpp:166 > > + if (m_document && (!elementToAnimationsMap().isEmpty() || !elementToCSSAnimationsMap().isEmpty() || !elementToCSSTransitionsMap().isEmpty())) { > > Make a hasAnimations() helper on AnimationTimeline Since these are specific to elements, I'm adding this protected method: bool hasElementAnimations() const { return !m_elementToAnimationsMap.isEmpty() || !m_elementToCSSAnimationsMap.isEmpty() || !m_elementToCSSTransitionsMap.isEmpty(); } > > Source/WebCore/style/StyleTreeResolver.cpp:333 > > + shouldRecompositeLayer = element.styleResolutionShouldRecompositeLayer() || shouldRecompositeLayer; > > use |= ? Will fix. > > LayoutTests/webanimations/css-animations.html:26 > > + test(() => { > > I like _ instead of () Noted, but I don't :) > > LayoutTests/webanimations/css-animations.html:44 > > + const animation = target.getAnimations()[0]; > > So does getAnimations force a style recalc? Yes, it calls updateStyleIfNeeded() on the document to ensure all pending CSS updates are accounted for before returning the current list of qualifying animations. > > LayoutTests/webanimations/css-animations.html:46 > > + assert_true(animation instanceof CSSAnimation, "The animation is a CSSAnimation."); > > + assert_true(animation instanceof Animation, "The animation is an Animation."); > > Should be the other way around Will fix. > > LayoutTests/webanimations/css-animations.html:66 > > + assert_equals(computedTiming.currentIteration, 1, "The animations's computed timing currentIteration property matches the properties set by CSS"); > > I assume this means the 2nd iteration? That's right, it's 0-based. > > LayoutTests/webanimations/css-animations.html:74 > > + assert_equals(computedTiming.progress, 0.5, "The animations's computed timing progress property matches the properties set by CSS"); > > Huh? Is this progress within the iteration? Correct. > > LayoutTests/webanimations/css-animations.html:81 > > + assert_equals(keyframes[0].offset, 0, "The animation's effect's first keyframe has a 0 offset."); > > Lucky we don't have a CSS property called offset. :) The spec says that the "offset" CSS property is referred to as cssOffset when in JS objects, so at least it prepared for that. > > LayoutTests/webanimations/css-animations.html:98 > > +targetTest(target => { > > + target.style.animationDelay = "-1s"; > > + target.style.animationDuration = "2s"; > > + target.style.animationName = "animation"; > > + > > + assert_equals(target.getAnimations()[0].effect.timing.delay, -1000, "The animation's delay matches the initial animation-delay property."); > > + > > + target.style.animationDelay = 0; > > + assert_equals(target.getAnimations()[0].effect.timing.delay, 0, "The animation's delay matches the updated animation-delay property."); > > +}, "Web Animations should reflect the animation-delay property."); > > Didn't you just test this? We just tested the initial value. I suppose there's some redundancy here. > > LayoutTests/webanimations/css-animations.html:144 > > + assert_equals(target.getAnimations()[0].effect.timing.iterations, 1, "The animation's duration matches the updated animation-iteration-count property."); > > Add one like this too: > > 189 assert_equals(target.getAnimations()[0], initialAnimation, "The > animation object remained the same instance throughout the test."); Will fix. > > LayoutTests/webanimations/css-animations.html:172 > > + assert_not_equals(updatedAnimation, initialAnimation, "Changing the animation-name property generates a different CSSAnimation object."); > > You should test this right after getting updatedAnimation OK. Thanks for taking the time to review all this code!
*** This bug has been marked as a duplicate of bug 183504 ***