We've added support for parsing easing values in webkit.org/b/181978, we now need to use those values when computing the progress and resolving keyframe effect values.
<rdar://problem/36866149>
Created attachment 332260 [details] Patch
Created attachment 332261 [details] Patch
Comment on attachment 332261 [details] Patch Attachment 332261 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6208343 New failing tests: transitions/default-timing-function.html
Created attachment 332266 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 332261 [details] Patch Attachment 332261 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6208379 New failing tests: transitions/default-timing-function.html
Created attachment 332270 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 332272 [details] Patch
Comment on attachment 332272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332272&action=review All good, but I think you need to use optionals instead of the -1s everywhere. We've had animation bugs before when using special values, because someone eventually uses them as a boolean. > Source/WebCore/ChangeLog:16 > + and identify cubic-bezier(0, 0, 0, 0) and cubic-bezier(1, 1, 1, 1) as linear timing functions, as called out > + by the WPT tests. Hmmm... the spec says this? I wonder what we do for our current animation code. > Source/WebCore/animation/AnimationEffect.cpp:283 > + bool goingFordwards = currentDirection() == AnimationEffect::ComputedDirection::Forwards; Fordwards > Source/WebCore/animation/KeyframeEffect.cpp:691 > +void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, double iterationProgress) There is a fair bit of work in this method. I wonder if we could cache some of the computed keyframe lists, rather than computing them every time iterationProgress changes. > Source/WebCore/animation/KeyframeEffect.cpp:700 > + // 1. If iteration progress is unresolved abort this procedure. I assume this is impossible here? > Source/WebCore/animation/KeyframeEffect.cpp:711 > + Vector<int> propertySpecificKeyframes; > + for (size_t i = 0; i < m_keyframes.size(); ++i) { Couldn't this be a Vector of keyframes, and use a for : loop? > Source/WebCore/animation/KeyframeEffect.cpp:720 > + propertySpecificKeyframes.append(i); Is there any point appending multiple keyframes with the same offset? e.g. 0 or 1 And if that case is already handled elsewhere, those can be bools rather than unsigned. > Source/WebCore/animation/KeyframeEffect.cpp:729 > + // offset of 0, a property value set to the neutral value for composition, and a composite operation of add, and prepend it to the beginning of > + // property-specific keyframes. It's a bit confusing that this last bit about setting up the default value happens much later. > Source/WebCore/animation/KeyframeEffect.cpp:731 > + propertySpecificKeyframes.insert(0, -1); I don't like this hack of inserting a -1. > Source/WebCore/animation/KeyframeEffect.cpp:750 > + // If iteration progress < 0 and there is more than one keyframe in property-specific keyframes with a computed keyframe offset of 0, > + // Add the first keyframe in property-specific keyframes to interval endpoints. > + intervalEndpoints.append(propertySpecificKeyframes.first()); So we probably could have handled this above by ignoring the subsequent zero keyframes and keeping a bool. > Source/WebCore/animation/KeyframeEffect.cpp:761 > + size_t indexOfLastKeyframeWithZeroOffset; Initialize this to 0. > Source/WebCore/animation/KeyframeEffect.cpp:771 > + double offset; > + if (keyframeIndex == -1) > + offset = i ? 1 : 0; > + else { > + auto& keyframe = m_keyframes[keyframeIndex]; > + offset = keyframe.key(); > + } Here's where you use the -1 hack. I hate to suggest this but I wonder if the propertySpecificKeyframes list could be indexed from 1. Or hold optionals? Also, there is a nice way to initialize offset immediately: auto offset = [&] () -> double { if (keyframeIndex == -1) return i ? 1 : 0; return m_keyframes[keyframeIndex].key(); }(); > Source/WebCore/animation/KeyframeEffect.cpp:780 > + if (indexOfFirstKeyframeToAddToIntervalEndpoints > -1) { >= 0 Again, I don't like the -1. Can you use an optional<unsigned>? > Source/WebCore/animation/KeyframeEffect.cpp:785 > + intervalEndpoints.append(propertySpecificKeyframes[indexOfFirstKeyframeToAddToIntervalEndpoints + 1]); > + } else { > + intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset]); > + intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset + 1]); Add ASSERT(indexOfLastKeyframeWithZeroOffset < propertySpecificKeyframes.size()) > Source/WebCore/animation/KeyframeEffect.cpp:795 > + auto keyframeStyle = keyframeIndex == -1 ? &targetStyle : m_keyframes[keyframeIndex].style(); Another -1. > Source/WebCore/platform/animation/TimingFunction.cpp:91 > + // decrement current step by one. Indent. > Source/WebCore/platform/animation/TimingFunction.h:167 > + bool isLinear() const > + { > + return (!m_x1 && !m_y1 && !m_x2 && !m_y2) || (m_x1 == 1.0 && m_y1 == 1.0 && m_x2 == 1.0 && m_y2 == 1.0); > + } It's also linear if 0, 0, 1, 1
Created attachment 332292 [details] Patch
(In reply to Dean Jackson from comment #9) > Comment on attachment 332272 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332272&action=review > > All good, but I think you need to use optionals instead of the -1s > everywhere. We've had animation bugs before when using special values, > because someone eventually uses them as a boolean. I've changed the two Vector<int> to Vector<std::optional<size_t>>. > > Source/WebCore/animation/AnimationEffect.cpp:283 > > + bool goingFordwards = currentDirection() == AnimationEffect::ComputedDirection::Forwards; > > Fordwards Fixed in new patch. > > Source/WebCore/animation/KeyframeEffect.cpp:691 > > +void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, double iterationProgress) > > There is a fair bit of work in this method. I wonder if we could cache some > of the computed keyframe lists, rather than computing them every time > iterationProgress changes. > > > Source/WebCore/animation/KeyframeEffect.cpp:700 > > + // 1. If iteration progress is unresolved abort this procedure. > > I assume this is impossible here? Yes, this would be caught at call sites. > > Source/WebCore/animation/KeyframeEffect.cpp:711 > > + Vector<int> propertySpecificKeyframes; > > + for (size_t i = 0; i < m_keyframes.size(); ++i) { > > Couldn't this be a Vector of keyframes, and use a for : loop? This would mean dropping KeyframeList which is a large undertaking that I'm considering but is ought of the scope of this patch. > > Source/WebCore/animation/KeyframeEffect.cpp:731 > > + propertySpecificKeyframes.insert(0, -1); > > I don't like this hack of inserting a -1. Using std::nullopt now. > > Source/WebCore/animation/KeyframeEffect.cpp:761 > > + size_t indexOfLastKeyframeWithZeroOffset; > > Initialize this to 0. Done. > > Source/WebCore/animation/KeyframeEffect.cpp:771 > > + double offset; > > + if (keyframeIndex == -1) > > + offset = i ? 1 : 0; > > + else { > > + auto& keyframe = m_keyframes[keyframeIndex]; > > + offset = keyframe.key(); > > + } > > Here's where you use the -1 hack. > > I hate to suggest this but I wonder if the propertySpecificKeyframes list > could be indexed from 1. Or hold optionals? > > Also, there is a nice way to initialize offset immediately: > > auto offset = [&] () -> double { > if (keyframeIndex == -1) > return i ? 1 : 0; > return m_keyframes[keyframeIndex].key(); > }(); Done, nice. > > Source/WebCore/animation/KeyframeEffect.cpp:785 > > + intervalEndpoints.append(propertySpecificKeyframes[indexOfFirstKeyframeToAddToIntervalEndpoints + 1]); > > + } else { > > + intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset]); > > + intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset + 1]); > > Add ASSERT(indexOfLastKeyframeWithZeroOffset < > propertySpecificKeyframes.size()) I added ASSERT(indexOfLastKeyframeWithZeroOffset < propertySpecificKeyframes.size() - 1); > > Source/WebCore/platform/animation/TimingFunction.cpp:91 > > + // decrement current step by one. > > Indent. Fixed. > > Source/WebCore/platform/animation/TimingFunction.h:167 > > + bool isLinear() const > > + { > > + return (!m_x1 && !m_y1 && !m_x2 && !m_y2) || (m_x1 == 1.0 && m_y1 == 1.0 && m_x2 == 1.0 && m_y2 == 1.0); > > + } > > It's also linear if 0, 0, 1, 1 Added that case too, nice.
Committed r227622: <https://trac.webkit.org/changeset/227622>