Summary: | [Web Animations] Mark promises as handled when rejected | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||
Component: | Animations | Assignee: | 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
Antoine Quint
2020-03-18 11:15:05 PDT
Created attachment 393958 [details]
Patch
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);
}
(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. Committed r258702: <https://trac.webkit.org/changeset/258702> All reviewed patches have been landed. Closing bug and clearing flags on attachment 393958 [details]. |