[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
bugzilla-daemon at webkit.org
Tue Mar 2 15:41:26 PST 2010
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.
More information about the webkit-reviews
mailing list