Bug 227915 – Port dialog close event to modern event handling code

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Tim Nguyen (:ntim)]

Use queueTaskToDispatchEvent()

[Radar WebKit Bug Importer]

<rdar://problem/80533217>

[Tim Nguyen (:ntim)]

Created attachment 433774 [details]
Patch

[Emilio Cobos Álvarez (:emilio)]

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?

[Tim Nguyen (:ntim)]

Created attachment 433779 [details]
Patch

[Tim Nguyen (:ntim)]

(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.

[Chris Dumez]

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).

[Ryosuke Niwa]

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.

[Tim Nguyen (:ntim)]

Created attachment 433818 [details]
Patch

[Chris Dumez]

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 }`

[Tim Nguyen (:ntim)]

Created attachment 433819 [details]
Patch

[EWS]

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].