Bug 78041 – Support reverse and alternate-reverse in CA animations

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dean Jackson]

https://bugs.webkit.org/show_bug.cgi?id=60525 introduces reverse and alternate-reverse. We'll need to support these in CA animations.

[Radar WebKit Bug Importer]

<rdar://problem/10822878>

[Dean Jackson]

Created attachment 138672 [details]
Patch

[Tim Horton]

Comment on attachment 138672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138672&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2033
> +    unsigned fromIndex = forwards ? 0 : 1;
> +    unsigned toIndex = forwards ? 1 : 0;

I assume this is here for the purposes of clarity, but it could be:

unsigned fromIndex = !forwards;
unsigned toIndex = forwards;

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2061
> +    bool forwards = (animation->direction() != Animation::AnimationDirectionReverse && animation->direction() != Animation::AnimationDirectionAlternateReverse);

You do this a lot; maybe it would be nice to factor it out somewhere, for readability?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:-2075
> -    // We toss the last tfArray value because it has to one shorter than the others.

It has to one shorter?

[Tim Horton]

Comment on attachment 138672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138672&action=review

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:145
> +        float ax = reverse ? 1 - static_cast<float>(ctf->x2()) : static_cast<float>(ctf->x1());
> +        float ay = reverse ? 1 - static_cast<float>(ctf->y2()) : static_cast<float>(ctf->y1());
> +        float bx = reverse ? 1 - static_cast<float>(ctf->x1()) : static_cast<float>(ctf->x2());
> +        float by = reverse ? 1 - static_cast<float>(ctf->y1()) : static_cast<float>(ctf->y2());

Could probably move the static_cast to the outside and cut it down by one per line.

[Simon Fraser (smfr)]

Comment on attachment 138672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138672&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2297
> +        if (i < (valueList.size() - 1)) {
> +            const TimingFunction* timingFunction;
> +            if (forwards)
> +                timingFunction = timingFunctionForAnimationValue(curValue, animation);
> +            else
> +                timingFunction = timingFunctionForAnimationValue(valueList.at(index - 1), animation);
> +            timingFunctions.append(timingFunction);

Looks like you have 3 copies of this code now. Can it be shared?

> LayoutTests/animations/animation-direction-reverse-fill-mode-hardware.html:82
> +          var computedValue = window.getComputedStyle(el).webkitTransform;
> +          var realValue = parseFloat(computedValue.split("(")[1].split(",")[4]);

This seems very similar to what animation-test-helpers.js already does. Can we share more code?

> LayoutTests/animations/animation-direction-reverse-fill-mode.html:75
> +            var realValue = window.getComputedStyle(el).getPropertyCSSValue("left").getFloatValue(CSSPrimitiveValue.CSS_NUMBER);

DItto.

> LayoutTests/animations/animation-direction-reverse-fill-mode.html:93
> +            var realValue = window.getComputedStyle(el).getPropertyCSSValue("left").getFloatValue(CSSPrimitiveValue.CSS_NUMBER);

Etc.

[Tim Horton]

Comment on attachment 138672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138672&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2296
> +            if (forwards)
> +                timingFunction = timingFunctionForAnimationValue(curValue, animation);
> +            else
> +                timingFunction = timingFunctionForAnimationValue(valueList.at(index - 1), animation);

This seems ripe for a ternary operator.

> LayoutTests/animations/animation-direction-reverse-fill-mode-hardware.html:68
> +            if (Math.abs(expectedValue - realValue) < allowance) {
> +              result += "PASS";
> +            } else {
> +              result += "FAIL";
> +            }

I don't know if we follow WebKit style in our JS, but we usually would omit those braces (and below in the same code). Also, I think the result += lines aren't sufficiently indented.

[Build Bot]

Comment on attachment 138672 [details]
Patch

Attachment 138672 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12528398

[Dean Jackson]

Created attachment 139287 [details]
Patch

[Dean Jackson]

(In reply to comment #8)
> Created an attachment (id=139287) [details]
> Patch

Includes all of Tim and Simon's review comments. I didn't quite reduce things as much as asked, because other refactorings made it less ugly anyway.

The big change since the previous patch is the large modification to animation-test-helpers, which allows the property getting/testing code to be shared.

[Tim Horton]

Comment on attachment 139287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139287&action=review

> LayoutTests/animations/animation-direction-reverse-fill-mode-hardware.html:54
> +            setTimeout(endTest, 0); // this set timeout should be ok in the test environment
> +                                    // since we're just giving style a chance to resolve

capitalization and punctuation (and "setTimeout"?)

> LayoutTests/animations/animation-direction-reverse-fill-mode-hardware.html:60
> +        for (var i=0; i < expectedValues.length; i++) {

space around the =

> LayoutTests/animations/animation-direction-reverse-fill-mode.html:67
> +            setTimeout(endTest, 0); // this set timeout should be ok in the test environment
> +                                    // since we're just giving style a chance to resolve

ditto

> LayoutTests/animations/animation-direction-reverse-fill-mode.html:73
> +        for (var i=0; i < expectedValues.length; i++) {

and here, and so on.

[Build Bot]

Comment on attachment 139287 [details]
Patch

Attachment 139287 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12559364

[Beth Dakin]

Comment on attachment 139287 [details]
Patch

It would be good to address Tim's most recent comments, and to try to get that Windows build working, but r=me!

[Tim Horton]

(In reply to comment #12)
> (From update of attachment 139287 [details])
> It would be good to address Tim's most recent comments, and to try to get that Windows build working, but r=me!

Windows error output is surprisingly annoying to read:

7>..\platform\graphics\ca\win\PlatformCAAnimationWin.cpp(292) : error C2511: 'void WebCore::PlatformCAAnimation::setTimingFunction(const WebCore::TimingFunction *)' : overloaded member function not found in 'WebCore::PlatformCAAnimation'
7>        c:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\PlatformCAAnimation.h(53) : see declaration of 'WebCore::PlatformCAAnimation'
7>..\platform\graphics\ca\win\PlatformCAAnimationWin.cpp(537) : error C2511: 'void WebCore::PlatformCAAnimation::setTimingFunctions(const WTF::Vector<T> &)' : overloaded member function not found in 'WebCore::PlatformCAAnimation'
7>        with
7>        [
7>            T=const WebCore::TimingFunction *
7>        ]
7>        c:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\ca\PlatformCAAnimation.h(53) : see declaration of 'WebCore::PlatformCAAnimation'

[Dean Jackson]

Created attachment 139329 [details]
Patch

[Dean Jackson]

Patch to check if windows compiles.

Followup bug: https://bugs.webkit.org/show_bug.cgi?id=85121

[Build Bot]

Comment on attachment 139329 [details]
Patch

Attachment 139329 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12559445

[Dean Jackson]

Created attachment 139332 [details]
Patch

[Dean Jackson]

http://trac.webkit.org/changeset/115540

Includes changes from Tim and Beth.