Bug 78041 - Support reverse and alternate-reverse in CA animations
Summary: Support reverse and alternate-reverse in CA animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-07 14:16 PST by Dean Jackson
Modified: 2012-04-27 22:11 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 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 Radar WebKit Bug Importer 2012-02-07 14:17:54 PST
<rdar://problem/10822878>
Comment 2 Dean Jackson 2012-04-24 15:47:32 PDT
Created attachment 138672 [details]
Patch
Comment 3 Tim Horton 2012-04-24 16:55:20 PDT
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?
Comment 4 Tim Horton 2012-04-24 17:00:10 PDT
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.
Comment 5 Simon Fraser (smfr) 2012-04-24 17:19:12 PDT
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.
Comment 6 Tim Horton 2012-04-24 17:34:18 PDT
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.
Comment 7 Build Bot 2012-04-24 17:45:26 PDT
Comment on attachment 138672 [details]
Patch

Attachment 138672 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12528398
Comment 8 Dean Jackson 2012-04-27 15:17:26 PDT
Created attachment 139287 [details]
Patch
Comment 9 Dean Jackson 2012-04-27 15:19:04 PDT
(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.
Comment 10 Tim Horton 2012-04-27 15:42:15 PDT
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.
Comment 11 Build Bot 2012-04-27 16:03:00 PDT
Comment on attachment 139287 [details]
Patch

Attachment 139287 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12559364
Comment 12 Beth Dakin 2012-04-27 17:05:32 PDT
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!
Comment 13 Tim Horton 2012-04-27 17:13:49 PDT
(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'
Comment 14 Dean Jackson 2012-04-27 20:34:23 PDT
Created attachment 139329 [details]
Patch
Comment 15 Dean Jackson 2012-04-27 20:35:22 PDT
Patch to check if windows compiles.

Followup bug: https://bugs.webkit.org/show_bug.cgi?id=85121
Comment 16 Build Bot 2012-04-27 20:56:05 PDT
Comment on attachment 139329 [details]
Patch

Attachment 139329 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12559445
Comment 17 Dean Jackson 2012-04-27 21:07:09 PDT
Created attachment 139332 [details]
Patch
Comment 18 Dean Jackson 2012-04-27 22:11:51 PDT
http://trac.webkit.org/changeset/115540

Includes changes from Tim and Beth.