[Web Animations] changing the timing of a transition such that it's finished should no longer have it marked as running

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] changing the effect of a transition should no longer have it marked as running

[Antoine Quint]

Created attachment 448244 [details]
Patch

[Dean Jackson]

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?

[Antoine Quint]

(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.

[Antoine Quint]

Committed r287550 (245685@trunk): <https://commits.webkit.org/245685@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87069661>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 234829

[Antoine Quint]

Created attachment 448257 [details]
Patch

[Darin Adler]

Where’s the explanation of why we had to revert? It says "caused a regression"?

[Antoine Quint]

(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.

[EWS]

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].