[webkit-reviews] review granted: [Bug 26869] Add fill modes for CSS Animations (don't reset state after the animation finishes) : [Attachment 49857] added convenience methods for testing fill mode

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 26869: Add fill modes for CSS Animations (don't reset state after the
animation finishes)
https://bugs.webkit.org/show_bug.cgi?id=26869

Attachment 49857: added convenience methods for testing fill mode
https://bugs.webkit.org/attachment.cgi?id=49857&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/LayoutTests/animations/fill-mode-transform.html
b/LayoutTests/animations/fill-mode-transform.html

> +	 setTimeout("snapshot('a', 0)", 100);
> +	 setTimeout("snapshot('a', 1)", 500);
> +	 setTimeout("snapshot('a', 2)", 1350);
> +	 setTimeout("snapshot('b', 3)", 100);
> +	 setTimeout("snapshot('b', 4)", 500);
> +	 setTimeout("snapshot('b', 5)", 1350);
> +	 setTimeout("snapshot('c', 6)", 100);
> +	 setTimeout("snapshot('c', 7)", 500);
> +	 setTimeout("snapshot('c', 8)", 1350);
> +	 setTimeout("snapshot('d', 9)", 100);
> +	 setTimeout("snapshot('d', 10)", 500);
> +	 setTimeout("snapshot('d', 11)", 1350);

A test like this will fail on the bots sporadically. We shouldn't add any new
animation tests that rely on setTimeout.

> diff --git a/WebCore/css/CSSComputedStyleDeclaration.cpp
b/WebCore/css/CSSComputedStyleDeclaration.cpp
> index 8039e35..407450c 100644
> --- a/WebCore/css/CSSComputedStyleDeclaration.cpp
> +++ b/WebCore/css/CSSComputedStyleDeclaration.cpp
> @@ -146,6 +146,7 @@ static const int computedProperties[] = {
>      CSSPropertyWebkitAnimationDelay,
>      CSSPropertyWebkitAnimationDirection,
>      CSSPropertyWebkitAnimationDuration,
> +    CSSPropertyWebkitAnimationFillMode,
>      CSSPropertyWebkitAnimationIterationCount,
>      CSSPropertyWebkitAnimationName,
>      CSSPropertyWebkitAnimationPlayState,
> @@ -1198,6 +1199,24 @@ PassRefPtr<CSSValue>
CSSComputedStyleDeclaration::getPropertyCSSValue(int proper
>	   }
>	   case CSSPropertyWebkitAnimationDuration:
>	       return getDurationValue(style->animations());
> +	   case CSSPropertyWebkitAnimationFillMode: {
> +	       RefPtr<CSSValueList> list =
CSSValueList::createCommaSeparated();
> +	       const AnimationList* t = style->animations();
> +	       if (t) {
> +		   for (size_t i = 0; i < t->size(); ++i) {
> +		       if (t->animation(i)->fillMode() ==
Animation::AnimationFillModeNone)
> +			  
list->append(CSSPrimitiveValue::createIdentifier(CSSValueNone));
> +		       else if (t->animation(i)->fillMode() ==
Animation::AnimationFillModeForwards)
> +			  
list->append(CSSPrimitiveValue::createIdentifier(CSSValueForwards));
> +		       else if (t->animation(i)->fillMode() ==
Animation::AnimationFillModeBackwards)
> +			  
list->append(CSSPrimitiveValue::createIdentifier(CSSValueBackwards));
> +		       else
> +			  
list->append(CSSPrimitiveValue::createIdentifier(CSSValueBoth));

Use a switch()?

> diff --git a/WebCore/css/CSSStyleSelector.cpp
b/WebCore/css/CSSStyleSelector.cpp

> +    CSSPrimitiveValue* primitiveValue =
static_cast<CSSPrimitiveValue*>(value);
> +    switch (primitiveValue->getIdent()) {
> +

Extra blank line here.

> diff --git a/WebCore/page/animation/AnimationBase.cpp
b/WebCore/page/animation/AnimationBase.cpp

>		   }
> +
>	       } else {

Extra line here.

> diff --git a/WebCore/page/animation/KeyframeAnimation.cpp
b/WebCore/page/animation/KeyframeAnimation.cpp

> @@ -233,12 +235,12 @@ void KeyframeAnimation::endAnimation()
>  #if USE(ACCELERATED_COMPOSITING)
>      if (m_object->hasLayer()) {
>	   RenderLayer* layer = toRenderBoxModelObject(m_object)->layer();
> -	   if (layer->isComposited())
> +	   if (layer->isComposited() && !m_animation->fillsForwards())
>	      
layer->backing()->animationFinished(m_keyframes.animationName());

Why are we calling endAnimation() if it fills fowrards?

> diff --git a/WebCore/platform/animation/Animation.h
b/WebCore/platform/animation/Animation.h

> @@ -119,6 +129,7 @@ private:
>      int m_iterationCount;
>      double m_delay;
>      double m_duration;
> +    AnimationFillMode m_fillMode : 2;

Put this next to the other bitfield member to optimize packing.

> diff --git a/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
b/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> index 22e39f5..9e79b8f 100644
> --- a/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> +++ b/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> @@ -1859,12 +1859,20 @@ void
GraphicsLayerCA::setupAnimation(CAPropertyAnimation* propertyAnim, const An
>      else if (anim->direction() == Animation::AnimationDirectionAlternate)
>	   repeatCount /= 2;
>  
> +    NSString* fillMode = kCAFillModeRemoved;
> +    if (anim->fillMode() == Animation::AnimationFillModeBackwards)
> +	  fillMode = kCAFillModeBackwards;
> +    else if (anim->fillMode() == Animation::AnimationFillModeForwards)
> +	  fillMode = kCAFillModeForwards;
> +    else if (anim->fillMode() == Animation::AnimationFillModeBoth)
> +	  fillMode = kCAFillModeBoth;

switch()?

r=me, if you can explain why calling endAnimation() is correct even when
filling forwards, and fix up the layout tests not rely on a lot of setTimeouts.