Bug 230404 – [Web Animations] Add support for composite operations for software animations

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

[Web Animations] Add support for composite operations for software animations

[Antoine Quint]

Created attachment 438469 [details]
Patch for EWS

[Antoine Quint]

Created attachment 438499 [details]
Patch for EWS

[Antoine Quint]

Created attachment 438651 [details]
Patch for EWS

[Antoine Quint]

Created attachment 438652 [details]
Patch for EWS

[Radar WebKit Bug Importer]

<rdar://problem/83495091>

[Antoine Quint]

Created attachment 441708 [details]
Patch for EWS

[Antoine Quint]

Created attachment 441716 [details]
Patch for EWS

[Antoine Quint]

Created attachment 441867 [details]
Patch for EWS

[Antoine Quint]

Created attachment 441868 [details]
Patch for EWS

[Antoine Quint]

Created attachment 441992 [details]
Patch for EWS

[Antoine Quint]

Created attachment 442025 [details]
Patch

[Antoine Quint]

Created attachment 442027 [details]
Patch

[Dean Jackson]

Comment on attachment 442027 [details]
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.

[Antoine Quint]

(In reply to Dean Jackson from comment #13)
> Comment on attachment 442027 [details]
> 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

Will fix in commit.
 
> Also, why not make this a static function so you don't need to duplicate it?

I'll definitely look into that.

> > 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? 

They are! Will fix in commit.

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1641
> > +        if (from != to)
> 
> Don't think you need this test.

Correct, will fix in commit.

[Antoine Quint]

Created attachment 442130 [details]
Patch for landing

[Antoine Quint]

(In reply to Antoine Quint from comment #14)
> (In reply to Dean Jackson from comment #13)
> > Comment on attachment 442027 [details]
> > 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;
> > 
> > Also, why not make this a static function so you don't need to duplicate it?
> 
> I'll definitely look into that.

I've looked into it and addressed it in the patch for landing.

[Antoine Quint]

Created attachment 442176 [details]
Patch for landing

[Antoine Quint]

Created attachment 443528 [details]
Patch for landing

[Antoine Quint]

Committed r285397 (243954@main): <https://commits.webkit.org/243954@main>

[Arcady Goldmints-Orlov]

*** Bug 224738 has been marked as a duplicate of this bug. ***