Bug 78041 - Support reverse and alternate-reverse in CA animations
: Support reverse and alternate-reverse in CA animations
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2012-02-07 14:16 PST by
Modified: 2012-04-27 22:11 PST (History)


Attachments
Patch (56.53 KB, patch)
2012-04-24 15:47 PST, Dean Jackson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.19 KB, patch)
2012-04-27 15:17 PST, Dean Jackson
bdakin: review+
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (70.10 KB, patch)
2012-04-27 20:34 PST, Dean Jackson
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (70.43 KB, patch)
2012-04-27 21:07 PST, Dean Jackson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-07 14:16:56 PST
https://bugs.webkit.org/show_bug.cgi?id=60525 introduces reverse and alternate-reverse. We'll need to support these in CA animations.
------- Comment #1 From 2012-02-07 14:17:54 PST -------
<rdar://problem/10822878>
------- Comment #2 From 2012-04-24 15:47:32 PST -------
Created an attachment (id=138672) [details]
Patch
------- Comment #3 From 2012-04-24 16:55:20 PST -------
(From update of attachment 138672 [details])
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?
------- Comment #4 From 2012-04-24 17:00:10 PST -------
(From update of attachment 138672 [details])
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.
------- Comment #5 From 2012-04-24 17:19:12 PST -------
(From update of attachment 138672 [details])
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.
------- Comment #6 From 2012-04-24 17:34:18 PST -------
(From update of attachment 138672 [details])
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.
------- Comment #7 From 2012-04-24 17:45:26 PST -------
(From update of attachment 138672 [details])
Attachment 138672 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12528398
------- Comment #8 From 2012-04-27 15:17:26 PST -------
Created an attachment (id=139287) [details]
Patch
------- Comment #9 From 2012-04-27 15:19:04 PST -------
(In reply to comment #8)
> Created an attachment (id=139287) [details] [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.
------- Comment #10 From 2012-04-27 15:42:15 PST -------
(From update of attachment 139287 [details])
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.
------- Comment #11 From 2012-04-27 16:03:00 PST -------
(From update of attachment 139287 [details])
Attachment 139287 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12559364
------- Comment #12 From 2012-04-27 17:05:32 PST -------
(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!
------- Comment #13 From 2012-04-27 17:13:49 PST -------
(In reply to comment #12)
> (From update of attachment 139287 [details] [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'
------- Comment #14 From 2012-04-27 20:34:23 PST -------
Created an attachment (id=139329) [details]
Patch
------- Comment #15 From 2012-04-27 20:35:22 PST -------
Patch to check if windows compiles.

Followup bug: https://bugs.webkit.org/show_bug.cgi?id=85121
------- Comment #16 From 2012-04-27 20:56:05 PST -------
(From update of attachment 139329 [details])
Attachment 139329 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12559445
------- Comment #17 From 2012-04-27 21:07:09 PST -------
Created an attachment (id=139332) [details]
Patch
------- Comment #18 From 2012-04-27 22:11:51 PST -------
http://trac.webkit.org/changeset/115540

Includes changes from Tim and Beth.