WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
227915
Port dialog close event to modern event handling code
https://bugs.webkit.org/show_bug.cgi?id=227915
Summary
Port dialog close event to modern event handling code
Tim Nguyen (:ntim)
Reported
2021-07-13 12:50:26 PDT
Use queueTaskToDispatchEvent()
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-07-13 12:51:02 PDT
<
rdar://problem/80533217
>
Tim Nguyen (:ntim)
Comment 2
2021-07-19 01:02:18 PDT
Created
attachment 433774
[details]
Patch
Emilio Cobos Álvarez (:emilio)
Comment 3
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?
Tim Nguyen (:ntim)
Comment 4
2021-07-19 02:30:22 PDT
Created
attachment 433779
[details]
Patch
Tim Nguyen (:ntim)
Comment 5
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.
Chris Dumez
Comment 6
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).
Ryosuke Niwa
Comment 7
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.
Tim Nguyen (:ntim)
Comment 8
2021-07-19 13:26:45 PDT
Created
attachment 433818
[details]
Patch
Chris Dumez
Comment 9
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 }`
Tim Nguyen (:ntim)
Comment 10
2021-07-19 13:35:29 PDT
Created
attachment 433819
[details]
Patch
EWS
Comment 11
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug