[webkit-reviews] review denied: [Bug 182098] [Web Animations] Account for provided easings when computing progress and resolving keyframe effect values : [Attachment 332272] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 25 10:00:14 PST 2018
Dean Jackson <dino at apple.com> has denied Antoine Quint <graouts at apple.com>'s
request for review:
Bug 182098: [Web Animations] Account for provided easings when computing
progress and resolving keyframe effect values
https://bugs.webkit.org/show_bug.cgi?id=182098
Attachment 332272: Patch
https://bugs.webkit.org/attachment.cgi?id=332272&action=review
--- Comment #9 from Dean Jackson <dino at apple.com> ---
Comment on attachment 332272
--> https://bugs.webkit.org/attachment.cgi?id=332272
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[indexOfFirstKeyframeToAddToI
ntervalEndpoints + 1]);
> + } else {
> +
intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroO
ffset]);
> +
intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroO
ffset + 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
More information about the webkit-reviews
mailing list