[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