Bug 209240

Summary: [Web Animations] Mark promises as handled when rejected
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, graouts, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 209241    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Description Antoine Quint 2020-03-18 11:15:05 PDT
We need to implement https://github.com/w3c/csswg-drafts/issues/4556. This will help with test flakiness so that we don't get console messages when calling cancel().
Comment 1 Radar WebKit Bug Importer 2020-03-18 11:15:35 PDT
<rdar://problem/60592305>
Comment 2 Antoine Quint 2020-03-19 02:51:03 PDT
Created attachment 393958 [details]
Patch
Comment 3 youenn fablet 2020-03-19 07:35:10 PDT
Comment on attachment 393958 [details]
Patch

Looks ok to me.

I still think we should not be passing a Handled value to reject() in this case though.
This promise is an attribute or state promise, reject should always be using Handled.
Instead of leaving the choice here, I would hardcode RejectAsHandled::Yes in DOMPromiseProxyWithResolveCallback<IDLType>::reject, something like:
inline void DOMPromiseProxyWithResolveCallback<IDLType>::reject(Exception exception)
{
    ASSERT(!m_valueOrException);

    m_valueOrException = ExceptionOr<void> { WTFMove(exception) };
    for (auto& deferredPromise : m_deferredPromises)
        deferredPromise->reject(m_valueOrException->exception(), RejectAsHandled::Yes);
}
Comment 4 Antoine Quint 2020-03-19 09:24:18 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 393958 [details]
> Patch
> 
> Looks ok to me.
> 
> I still think we should not be passing a Handled value to reject() in this
> case though.
> This promise is an attribute or state promise, reject should always be using
> Handled.
> Instead of leaving the choice here, I would hardcode RejectAsHandled::Yes in
> DOMPromiseProxyWithResolveCallback<IDLType>::reject, something like:
> inline void DOMPromiseProxyWithResolveCallback<IDLType>::reject(Exception
> exception)
> {
>     ASSERT(!m_valueOrException);
> 
>     m_valueOrException = ExceptionOr<void> { WTFMove(exception) };
>     for (auto& deferredPromise : m_deferredPromises)
>         deferredPromise->reject(m_valueOrException->exception(),
> RejectAsHandled::Yes);
> }

I'm landing this patch as-is, I don't feel like we should make that kind of broader change without some standards discussion first.
Comment 5 EWS 2020-03-19 09:42:29 PDT
Committed r258702: <https://trac.webkit.org/changeset/258702>

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