Bug 230404 - [Web Animations] Add support for composite operations for software animations
Summary: [Web Animations] Add support for composite operations for software animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
: 224738 (view as bug list)
Depends on: 230347 232069
Blocks: 189299 232081 232082 232084 232085 232087 232086
  Show dependency treegraph
 
Reported: 2021-09-17 07:00 PDT by Antoine Quint
Modified: 2021-11-30 07:56 PST (History)
21 users (show)

See Also:


Attachments
Patch for EWS (786.75 KB, patch)
2021-09-17 07:00 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (789.28 KB, patch)
2021-09-17 12:08 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (797.79 KB, patch)
2021-09-20 03:13 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (797.67 KB, patch)
2021-09-20 03:24 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (789.69 KB, patch)
2021-10-19 03:52 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (792.21 KB, patch)
2021-10-19 06:04 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (794.04 KB, patch)
2021-10-20 04:49 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (792.69 KB, patch)
2021-10-20 04:57 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (801.16 KB, patch)
2021-10-21 00:18 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (780.58 KB, patch)
2021-10-21 08:16 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (780.71 KB, patch)
2021-10-21 08:25 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff
Patch for landing (780.91 KB, patch)
2021-10-22 00:52 PDT, Antoine Quint
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (784.51 KB, patch)
2021-10-22 10:23 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (784.81 KB, patch)
2021-11-08 00:32 PST, Antoine Quint
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***