Bug 230404

Summary: [Web Animations] Add support for composite operations for software animations
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, dino, dpino, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, keith_miller, kondapallykalyan, macpherson, mark.lam, menard, mifenton, mmaxfield, msaboff, nmouchtaris, pdr, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 230347, 232069    
Bug Blocks: 232081, 232082, 232085, 232087, 189299, 232084, 232086    
Attachments:
Description Flags
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch for EWS
none
Patch
none
Patch
dino: review+
Patch for landing
ews-feeder: commit-queue-
Patch for landing
none
Patch for landing ews-feeder: commit-queue-

Description Antoine Quint 2021-09-17 07:00:03 PDT
[Web Animations] Add support for composite operations for software animations
Comment 1 Antoine Quint 2021-09-17 07:00:55 PDT
Created attachment 438469 [details]
Patch for EWS
Comment 2 Antoine Quint 2021-09-17 12:08:20 PDT
Created attachment 438499 [details]
Patch for EWS
Comment 3 Antoine Quint 2021-09-20 03:13:54 PDT
Created attachment 438651 [details]
Patch for EWS
Comment 4 Antoine Quint 2021-09-20 03:24:44 PDT
Created attachment 438652 [details]
Patch for EWS
Comment 5 Radar WebKit Bug Importer 2021-09-24 07:01:25 PDT
<rdar://problem/83495091>
Comment 6 Antoine Quint 2021-10-19 03:52:03 PDT
Created attachment 441708 [details]
Patch for EWS
Comment 7 Antoine Quint 2021-10-19 06:04:47 PDT
Created attachment 441716 [details]
Patch for EWS
Comment 8 Antoine Quint 2021-10-20 04:49:54 PDT
Created attachment 441867 [details]
Patch for EWS
Comment 9 Antoine Quint 2021-10-20 04:57:17 PDT
Created attachment 441868 [details]
Patch for EWS
Comment 10 Antoine Quint 2021-10-21 00:18:31 PDT
Created attachment 441992 [details]
Patch for EWS
Comment 11 Antoine Quint 2021-10-21 08:16:25 PDT
Created attachment 442025 [details]
Patch
Comment 12 Antoine Quint 2021-10-21 08:25:32 PDT
Created attachment 442027 [details]
Patch
Comment 13 Dean Jackson 2021-10-21 08:58:00 PDT
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.
Comment 14 Antoine Quint 2021-10-21 11:29:59 PDT
(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.
Comment 15 Antoine Quint 2021-10-22 00:52:03 PDT
Created attachment 442130 [details]
Patch for landing
Comment 16 Antoine Quint 2021-10-22 00:53:01 PDT
(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.
Comment 17 Antoine Quint 2021-10-22 10:23:42 PDT
Created attachment 442176 [details]
Patch for landing
Comment 18 Antoine Quint 2021-11-08 00:32:47 PST
Created attachment 443528 [details]
Patch for landing
Comment 19 Antoine Quint 2021-11-08 03:35:11 PST
Committed r285397 (243954@main): <https://commits.webkit.org/243954@main>
Comment 20 Arcady Goldmints-Orlov 2021-11-30 07:56:53 PST
*** Bug 224738 has been marked as a duplicate of this bug. ***