[webkit-reviews] review denied: [Bug 236019] Overlap computations are broken with rotate, translate, scale properties : [Attachment 450665] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 236019: Overlap computations are broken with rotate, translate, scale
properties
https://bugs.webkit.org/show_bug.cgi?id=236019

Attachment 450665: Patch

https://bugs.webkit.org/attachment.cgi?id=450665&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 450665
  --> https://bugs.webkit.org/attachment.cgi?id=450665
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450665&action=review

Seems like this needs a a bit more improvement to address Simon’s feedback on
tests to make sure we cover more complex cases correctly.

> Source/WebCore/animation/DocumentTimeline.cpp:351
> +	   auto& animatedProperties = effect->animatedProperties();

In this narrow context could use a one word name, properties.

> Source/WebCore/animation/DocumentTimeline.cpp:352
> +	   if (animatedProperties.contains(CSSPropertyTransform)

All these separate calls to contains are awkward. Consider refactoring that
make it easier to see this is correct. Maybe just add a contains that takes a
Span and pass them all with { }. Could also have this list of property names be
a names constexpr array we can use over and over again. Or a function that
returns a span over that kind of array.

>> Source/WebCore/animation/DocumentTimeline.cpp:356
>> +		result = result ||
effect->computeExtentOfTransformAnimation(bounds);
> 
> You need to be careful: the || operator can short-circuit.

As Simon says, seems highly likely this is wrong. If we need to call
computeExtentOfTransformAnimation on all the effects then it needs to be
outside the || context.

Would be good to add a test case complex enough to show this.

> Source/WebCore/animation/DocumentTimeline.cpp:380
> +	       if
(keyframeEffect->isCurrentlyAffectingProperty(CSSPropertyTransform,
KeyframeEffect::Accelerated::Yes)

Since we see the same set of properties here the same considerations as above
apply. Find a way to have the code easier to read and clearer we didn’t it’s
forget one property.

> Source/WebCore/animation/KeyframeEffect.cpp:1921
> +    ASSERT(m_blendingKeyframes.containsProperty(CSSPropertyTransform)

And the pattern recurs again here.