Bug 203087

<rdar://problem/56744401>

Created attachment 382805 [details] Patch

Comment on attachment 382805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382805&action=review > Source/WebCore/Modules/applepay/ApplePaySession.cpp:831 > +bool ApplePaySession::canSuspendWhileActive() const I find the naming very confusing. How about something like "needsAbortingOnSuspension" ? > Source/WebCore/Modules/applepay/ApplePaySession.cpp:838 > + case State::SuspendedWhileActive: Do we really need a new state? Why not go directly to canceled? > Source/WebCore/Modules/applepay/ApplePaySession.cpp:860 > + // FIXME: Is TaskSource::UserInteraction correct here? I know but it is not the same spec but UserInteraction is what's used in this spec at least: https://w3c.github.io/payment-request/ > Source/WebCore/Modules/applepay/ApplePaySession.cpp:861 > + context.eventLoop().queueTask(TaskSource::UserInteraction, context, [this, pendingActivity = makePendingActivity(*this)] { Why not queue the task in the suspend()? Since you're using the event loop, it would fire until you resume anyway. This way, you don't need a resume() method. Also, you should consider using ActiveDOMObject::queueTaskKeepingObjectAlive() instead to queue the task.

(In reply to Chris Dumez from comment #3) > Comment on attachment 382805 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382805&action=review > > > Source/WebCore/Modules/applepay/ApplePaySession.cpp:831 > > +bool ApplePaySession::canSuspendWhileActive() const > > I find the naming very confusing. How about something like > "needsAbortingOnSuspension" ? > > > Source/WebCore/Modules/applepay/ApplePaySession.cpp:838 > > + case State::SuspendedWhileActive: > > Do we really need a new state? Why not go directly to canceled? > > > Source/WebCore/Modules/applepay/ApplePaySession.cpp:860 > > + // FIXME: Is TaskSource::UserInteraction correct here? > > I know but it is not the same spec but UserInteraction is what's used in > this spec at least: https://w3c.github.io/payment-request/ > > > Source/WebCore/Modules/applepay/ApplePaySession.cpp:861 > > + context.eventLoop().queueTask(TaskSource::UserInteraction, context, [this, pendingActivity = makePendingActivity(*this)] { > > Why not queue the task in the suspend()? Since you're using the event loop, > it would fire until you resume anyway. This way, you don't need a resume() > method. it would *NOT* fire until you resume anyway

Created attachment 382806 [details] Patch

Comment on attachment 382806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382806&action=review > Source/WebCore/Modules/applepay/ApplePaySession.cpp:866 > + queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, ApplePayCancelEvent::create(eventNames().cancelEvent, { })); nice.

Comment on attachment 382806 [details] Patch Clearing flags on attachment: 382806 Committed r252057: <https://trac.webkit.org/changeset/252057>

All reviewed patches have been landed. Closing bug.