Bug 183161 – [Web Animations] Correct implementation of pending tasks and promises

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Web Animations] Correct implementation of pending tasks and promises

[Antoine Quint]

Created attachment 334685 [details]
Patch

[Dean Jackson]

Comment on attachment 334685 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:942
> +                document->postTask([this] (auto&) {
> +                    if (hasPendingPauseTask() && m_timeline)
> +                        runPendingPauseTask();
> +                });

I don't understand the build errors here, sorry. You could try [protectedThis = makeRef(*this)] and then protectedThis->hasPendingPauseTask()

> Source/WebCore/animation/WebAnimation.h:149
> +    UniqueRef<ReadyPromise> m_readyPromise;
> +    UniqueRef<FinishedPromise> m_finishedPromise;

I didn't really understand the message from the changelog about this. Why store refs, instead of the promises directly? Then instead of clear(), you'd call clear and assign a new instance.

[Chris Dumez]

Comment on attachment 334685 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:952
> +                    if (hasPendingPlayTask() && m_timeline)

Use this->hasPendingPlayTask() to make gcc happy.

[Chris Dumez]

Comment on attachment 334685 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:939
> +                document->postTask([this] (auto&) {

What guarantees |this| is still alive when the task is executed asynchronously? We usually capture protectedThis = makeRef(*this) to avoid this problem.

>> Source/WebCore/animation/WebAnimation.cpp:942
>> +                });
> 
> I don't understand the build errors here, sorry. You could try [protectedThis = makeRef(*this)] and then protectedThis->hasPendingPauseTask()

gcc just needs some explicit this-> to make it happy.

> Source/WebCore/animation/WebAnimation.cpp:949
> +        if (is<DocumentTimeline>(m_timeline)) {

This could be merged with the previous condition (is<>() does a null check when passed in a pointer type.
if (hasPendingPlayTask() && is<DocumentTimeline>(m_timeline))

> Source/WebCore/animation/WebAnimation.cpp:951
> +                document->postTask([this] (auto&) {

Ditto about lifetime.

[Antoine Quint]

Created attachment 334696 [details]
Patch

[Antoine Quint]

Committed r229069: <https://trac.webkit.org/changeset/229069>

[Radar WebKit Bug Importer]

<rdar://problem/37953727>