[webkit-reviews] review granted: [Bug 230404] [Web Animations] Add support for composite operations for software animations : [Attachment 442027] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 230404: [Web Animations] Add support for composite operations for software
animations
https://bugs.webkit.org/show_bug.cgi?id=230404

Attachment 442027: Patch

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




--- Comment #13 from Dean Jackson <dino at apple.com> ---
Comment on attachment 442027
  --> https://bugs.webkit.org/attachment.cgi?id=442027
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1343
> +		       shadow = shadow ? shadow->next() : 0;

nullptr instead of 0

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1388
> +		       shadow = shadow ? shadow->next() : 0;

ditto

Also, why not make this a static function so you don't need to duplicate it?

> Source/WebCore/platform/animation/AnimationUtilities.h:57
> +	   return static_cast<unsigned>(lround(to > from ?
static_cast<double>(from) + static_cast<double>(to - from) * context.progress :
static_cast<double>(from) - static_cast<double>(from - to) *
context.progress));

to > from ? from + (to-from) * p : from - (from-to) * p
to > from ? from + to*p - from*p : from - from*p + to*p
to > from ? from + to*p - from*p : from + to*p - from*p

Aren't both sides the same? 

This explains why you don't check for to > from in the other blends.

> Source/WebCore/platform/graphics/transforms/AffineTransform.cpp:387
> +    if (compositeOperation != CompositeOperation::Replace) {

I think it might be more understandable if you explicitly test for Add or
Accumulate

> Source/WebCore/platform/graphics/transforms/RotateTransformOperation.cpp:63
> -	   FloatPoint3D vector { static_cast<float>(op.m_x),
static_cast<float>(op.m_y), static_cast<float>(op.m_z) };
> -	   vector.normalize();
> -	   return vector;
> +	   auto length = std::hypot(op.m_x, op.m_y, op.m_z);
> +	   return { static_cast<float>(op.m_x / length),
static_cast<float>(op.m_y / length), static_cast<float>(op.m_z / length) };

Seems unrelated, but fine.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1641
> +	   if (from != to)

Don't think you need this test.