[webkit-reviews] review granted: [Bug 183558] [Web Animations] Create, update and remove CSSAnimation and CSSTransition objects on the DocumentTimeline : [Attachment 335572] 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 183558: [Web Animations] Create, update and remove CSSAnimation and
CSSTransition objects on the DocumentTimeline
https://bugs.webkit.org/show_bug.cgi?id=183558

Attachment 335572: Patch

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




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