Use queueTaskToDispatchEvent()
<rdar://problem/80533217>
Created attachment 433774 [details] Patch
Comment on attachment 433774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433774&action=review > Source/WebCore/html/HTMLDialogElement.h:55 > + const char* activeDOMObjectName() const final { return "HTMLMarqueeElement"; } Copy-pasta from marquee?
Created attachment 433779 [details] Patch
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3) > Comment on attachment 433774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433774&action=review > > > Source/WebCore/html/HTMLDialogElement.h:55 > > + const char* activeDOMObjectName() const final { return "HTMLMarqueeElement"; } > > Copy-pasta from marquee? Good catch, thanks! I originally did change it, but looks like I accidentally undid the change.
Comment on attachment 433779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433779&action=review > Source/WebCore/html/HTMLDialogElement.cpp:100 > + queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No)); I think we should just call: ``` document().eventLoop().queueTask(TaskSource::UserInteraction, [protectedThis = makeRef(*this)] { protectedThis->dispatchEvent(Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No)); }); ``` > Source/WebCore/html/HTMLDialogElement.h:33 > +class HTMLDialogElement final : public HTMLElement, public ActiveDOMObject { Subclasses of Node usually do not subclass ActiveDOMObject. ActiveDOMObject is used to manage the lifetime of the JS wrapper but Node has its own wrapper lifetime management in JSNodeCustom.cpp (wrapper stays alive as long as the node is connected + some other conditions).
Comment on attachment 433779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433779&action=review >> Source/WebCore/html/HTMLDialogElement.cpp:100 >> + queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No)); > > I think we should just call: > ``` > document().eventLoop().queueTask(TaskSource::UserInteraction, [protectedThis = makeRef(*this)] { > protectedThis->dispatchEvent(Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No)); > }); > ``` We need to use GCReachableRef instead since the JS wrapper will be collected by JS if we just use Ref.
Created attachment 433818 [details] Patch
Comment on attachment 433818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433818&action=review r=me with nit. Please make sure the bots are happy before landing. > Source/WebCore/html/HTMLDialogElement.cpp:93 > + document().eventLoop().queueTask(TaskSource::UserInteraction, [protectedThis = GCReachableRef<HTMLDialogElement>(*this)] { With C++17, you can probably write: `protectedThis = GCReachableRef { *this }`
Created attachment 433819 [details] Patch
Committed r280049 (239784@main): <https://commits.webkit.org/239784@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433819 [details].