Bug 227915 - Port dialog close event to modern event handling code
Summary: Port dialog close event to modern event handling code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on: 227493
Blocks: dialog-element 202843 228846
  Show dependency treegraph
 
Reported: 2021-07-13 12:50 PDT by Tim Nguyen (:ntim)
Modified: 2021-09-18 15:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.32 KB, patch)
2021-07-19 01:02 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (4.32 KB, patch)
2021-07-19 02:30 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (3.73 KB, patch)
2021-07-19 13:26 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (3.71 KB, patch)
2021-07-19 13:35 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Nguyen (:ntim) 2021-07-13 12:50:26 PDT
Use queueTaskToDispatchEvent()
Comment 1 Radar WebKit Bug Importer 2021-07-13 12:51:02 PDT
<rdar://problem/80533217>
Comment 2 Tim Nguyen (:ntim) 2021-07-19 01:02:18 PDT
Created attachment 433774 [details]
Patch
Comment 3 Emilio Cobos Álvarez (:emilio) 2021-07-19 01:32:19 PDT
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?
Comment 4 Tim Nguyen (:ntim) 2021-07-19 02:30:22 PDT
Created attachment 433779 [details]
Patch
Comment 5 Tim Nguyen (:ntim) 2021-07-19 02:31:11 PDT
(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 6 Chris Dumez 2021-07-19 08:29:08 PDT
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 7 Ryosuke Niwa 2021-07-19 10:42:13 PDT
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.
Comment 8 Tim Nguyen (:ntim) 2021-07-19 13:26:45 PDT
Created attachment 433818 [details]
Patch
Comment 9 Chris Dumez 2021-07-19 13:30:16 PDT
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 }`
Comment 10 Tim Nguyen (:ntim) 2021-07-19 13:35:29 PDT
Created attachment 433819 [details]
Patch
Comment 11 EWS 2021-07-19 15:01:10 PDT
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].