Bug 234823 - [Web Animations] changing the timing of a transition such that it's finished should no longer have it marked as running
Summary: [Web Animations] changing the timing of a transition such that it's finished ...
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, WebExposed
Depends on: 234829
Blocks: 235130
  Show dependency tree / graph
 
Reported: 2022-01-03 08:40 PST by Antoine Quint
Modified: 2022-01-12 12:38 PST (History)
5 users (show)

See Also:

Attachments
Patch (5.10 KB, patch)
2022-01-03 08:40 PST, Antoine Quint 		no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2022-01-03 14:25 PST, Antoine Quint 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2022-01-03 08:40:17 PST 
[Web Animations] changing the effect of a transition should no longer have it marked as running
Comment 1 Antoine Quint 2022-01-03 08:40:51 PST 
Created attachment 448244 [details]
Patch
Comment 2 Dean Jackson 2022-01-03 08:43:05 PST 
Comment on attachment 448244 [details]
Patch

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

> Source/WebCore/animation/DeclarativeAnimation.cpp:242
> +    auto effectChanged = effect() != newEffect;
> +
> +    WebAnimation::setEffect(WTFMove(newEffect));

Why would you call setEffect if !effectChanged?
Comment 3 Antoine Quint 2022-01-03 08:44:49 PST 
(In reply to Dean Jackson from comment #2)
> Comment on attachment 448244 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448244&action=review
> 
> > Source/WebCore/animation/DeclarativeAnimation.cpp:242
> > +    auto effectChanged = effect() != newEffect;
> > +
> > +    WebAnimation::setEffect(WTFMove(newEffect));
> 
> Why would you call setEffect if !effectChanged?

Yeah, I suppose we could return early here, but the idea was that it made no assumption as to what WebAnimation::setEffect() did with that value.
Comment 4 Antoine Quint 2022-01-03 10:49:37 PST 
Committed r287550 (245685@trunk): <https://commits.webkit.org/245685@trunk>
Comment 5 Radar WebKit Bug Importer 2022-01-03 10:50:22 PST 
<rdar://problem/87069661>
Comment 6 WebKit Commit Bot 2022-01-03 13:45:07 PST 
Re-opened since this is blocked by bug 234829
Comment 7 Antoine Quint 2022-01-03 14:25:33 PST 
Created attachment 448257 [details]
Patch
Comment 8 Darin Adler 2022-01-03 14:44:13 PST 
Where’s the explanation of why we had to revert? It says "caused a regression"?
Comment 9 Antoine Quint 2022-01-03 14:59:55 PST 
(In reply to Darin Adler from comment #8)
> Where’s the explanation of why we had to revert? It says "caused a
> regression"?

This was due not knowing how to use webkitbot correctly on the slack channel.

The reason the original patch was wrong was that it regressed another sub test in this WPT test.
Comment 10 EWS 2022-01-04 08:12:30 PST 
Committed r287568 (245703@main): <https://commits.webkit.org/245703@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448257 [details].