https://bugs.webkit.org/show_bug.cgi?id=60525 introduces reverse and alternate-reverse. We'll need to support these in CA animations.
<rdar://problem/10822878>
Created attachment 138672 [details] Patch
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 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 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 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 on attachment 138672 [details] Patch Attachment 138672 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12528398
Created attachment 139287 [details] Patch
(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 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 on attachment 139287 [details] Patch Attachment 139287 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12559364
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!
(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'
Created attachment 139329 [details] Patch
Patch to check if windows compiles. Followup bug: https://bugs.webkit.org/show_bug.cgi?id=85121
Comment on attachment 139329 [details] Patch Attachment 139329 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12559445
Created attachment 139332 [details] Patch
http://trac.webkit.org/changeset/115540 Includes changes from Tim and Beth.