Bug 203714

Summary:

Add a helper function to schedule a task to dispatch an event to ActiveDOMObject

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, kangil.han, ryuan.choi, sergio, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

203680

Bug Blocks:

203667

Attachments:

Description	Flags
WIP	none
WIP2	none
WIP3	none
WIP4	none
WIP5	none
WIP6	none
WIP7	none
Patch	none
Patch for landing	none
Patch for landing	none

Ryosuke Niwa

Reported 2019-10-31 19:17:06 PDT

Refactor a helper function added in https://bugs.webkit.org/show_bug.cgi?id=203680 to ActiveDOMObject.

Attachments
WIP (14.58 KB, patch) 2019-11-01 16:05 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP2 (14.58 KB, patch) 2019-11-01 16:08 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP3 (14.88 KB, patch) 2019-11-01 16:13 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP4 (14.78 KB, patch) 2019-11-01 16:39 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP5 (14.79 KB, patch) 2019-11-01 16:44 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP6 (14.64 KB, patch) 2019-11-01 16:46 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP7 (14.66 KB, patch) 2019-11-01 16:50 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch (18.66 KB, patch) 2019-11-01 17:17 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (16.94 KB, patch) 2019-11-03 00:31 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (16.97 KB, patch) 2019-11-03 00:31 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2019-11-01 16:05:01 PDT

Created attachment 382643 [details] WIP

Ryosuke Niwa

Comment 2 2019-11-01 16:08:03 PDT

Created attachment 382645 [details] WIP2

Ryosuke Niwa

Comment 3 2019-11-01 16:13:46 PDT

Created attachment 382646 [details] WIP3

Chris Dumez

Comment 4 2019-11-01 16:24:58 PDT

Comment on attachment 382646 [details] WIP3 View in context: https://bugs.webkit.org/attachment.cgi?id=382646&action=review We use Function instead of WTF::Function. > Source/WebCore/dom/ActiveDOMObject.h:122 > + void queueTaskToDispatchEventOnThis(EventTargetType& target, TaskSource source, Ref<EventType>&& event, WTF::Function<void (EventTargetType&)>&& postDispatchWork = { }) Maybe this could be static since we already pass the target as parameter? Then we don't need "OnThis" in the name. > Source/WebCore/dom/ActiveDOMObject.h:134 > + void queueTaskKeepingThisObjectAlive(T& thisObject, TaskSource source, WTF::Function<void ()>&& task) Ditto, could this be static? > Source/WebCore/dom/ActiveDOMObject.h:146 > + void queueTaskInEventLoop(TaskSource, WTF::Function<void ()>&&); That one is fine as non-static.

Chris Dumez

Comment 5 2019-11-01 16:30:13 PDT

Comment on attachment 382646 [details] WIP3 View in context: https://bugs.webkit.org/attachment.cgi?id=382646&action=review >> Source/WebCore/dom/ActiveDOMObject.h:122 >> + void queueTaskToDispatchEventOnThis(EventTargetType& target, TaskSource source, Ref<EventType>&& event, WTF::Function<void (EventTargetType&)>&& postDispatchWork = { }) > > Maybe this could be static since we already pass the target as parameter? Then we don't need "OnThis" in the name. Maybe this could be simply queueEvent() ? > Source/WebCore/dom/ActiveDOMObject.h:126 > + queueTaskInEventLoop(source, [protectedTarget = makeRef(target), movedEvent = WTFMove(event), doWork = WTFMove(postDispatchWork), activity = makePendingActivity(target)] () { Seems like maybe this could use your queueTaskKeepingThisObjectAlive() implementation? It does keep this alive. >> Source/WebCore/dom/ActiveDOMObject.h:134 >> + void queueTaskKeepingThisObjectAlive(T& thisObject, TaskSource source, WTF::Function<void ()>&& task) > > Ditto, could this be static? I would have just called this queueTask() (with the other being queueEvent()). This name really is super long, although it has the benefit to be very clear :)

Ryosuke Niwa

Comment 6 2019-11-01 16:33:05 PDT

(In reply to Chris Dumez from comment #4) > Comment on attachment 382646 [details] > WIP3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382646&action=review > > We use Function instead of WTF::Function. > > > Source/WebCore/dom/ActiveDOMObject.h:122 > > + void queueTaskToDispatchEventOnThis(EventTargetType& target, TaskSource source, Ref<EventType>&& event, WTF::Function<void (EventTargetType&)>&& postDispatchWork = { }) > > Maybe this could be static since we already pass the target as parameter? > Then we don't need "OnThis" in the name. We can do that. > > Source/WebCore/dom/ActiveDOMObject.h:134 > > + void queueTaskKeepingThisObjectAlive(T& thisObject, TaskSource source, WTF::Function<void ()>&& task) > > Ditto, could this be static? Sure.

Chris Dumez

Comment 7 2019-11-01 16:33:52 PDT

And BTW, I think those are going to be super helpful.

Ryosuke Niwa

Comment 8 2019-11-01 16:38:25 PDT

(In reply to Chris Dumez from comment #5) > Comment on attachment 382646 [details] > WIP3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382646&action=review > > >> Source/WebCore/dom/ActiveDOMObject.h:122 > >> + void queueTaskToDispatchEventOnThis(EventTargetType& target, TaskSource source, Ref<EventType>&& event, WTF::Function<void (EventTargetType&)>&& postDispatchWork = { }) > > > > Maybe this could be static since we already pass the target as parameter? Then we don't need "OnThis" in the name. > > Maybe this could be simply queueEvent() ? Hm... I'd keep the verbose name for now because it's too confusing with other event queues and scoped events, etc... Once we get rid of those confusing things, we can rename it to a something simple. > > Source/WebCore/dom/ActiveDOMObject.h:126 > > + queueTaskInEventLoop(source, [protectedTarget = makeRef(target), movedEvent = WTFMove(event), doWork = WTFMove(postDispatchWork), activity = makePendingActivity(target)] () { > > Seems like maybe this could use your queueTaskKeepingThisObjectAlive() > implementation? It does keep this alive. Indeed.

Ryosuke Niwa

Comment 9 2019-11-01 16:39:01 PDT

Created attachment 382650 [details] WIP4

Chris Dumez

Comment 10 2019-11-01 16:42:49 PDT

Comment on attachment 382650 [details] WIP4 View in context: https://bugs.webkit.org/attachment.cgi?id=382650&action=review > Source/WebCore/dom/ActiveDOMObject.h:131 > + static void queueTaskToDispatchEvent(EventTargetType& target, TaskSource source, Ref<EventType>&& event, Function<void (EventTargetType&)>&& postDispatchWork = { }) I am personally not convinced yet of the value of the last postDispatchWork parameter. As a developer, it feels like if I had to do some other work besides dispatching the event, I'd use queueTaskKeepingThisObjectAlive() and do the dispatch & other work myself. Also, why would you get a postWork but no preWork? > Source/WebCore/dom/ActiveDOMObject.h:134 > + queueTaskToDispatchEvent(target, source, [target, doWork = WTFMove(postDispatchWork)] () { I think you meant queueTaskKeepingThisObjectAlive ?

Ryosuke Niwa

Comment 11 2019-11-01 16:44:11 PDT

Created attachment 382651 [details] WIP5

Ryosuke Niwa

Comment 12 2019-11-01 16:46:06 PDT

(In reply to Chris Dumez from comment #10) > Comment on attachment 382650 [details] > WIP4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382650&action=review > > > Source/WebCore/dom/ActiveDOMObject.h:131 > > + static void queueTaskToDispatchEvent(EventTargetType& target, TaskSource source, Ref<EventType>&& event, Function<void (EventTargetType&)>&& postDispatchWork = { }) > > I am personally not convinced yet of the value of the last postDispatchWork > parameter. As a developer, it feels like if I had to do some other work > besides dispatching the event, I'd use queueTaskKeepingThisObjectAlive() and > do the dispatch & other work myself. Also, why would you get a postWork but > no preWork? That's a good point. Now that we have queueTaskKeepingThisObjectAlive, we probably don't need this. > > Source/WebCore/dom/ActiveDOMObject.h:134 > > + queueTaskToDispatchEvent(target, source, [target, doWork = WTFMove(postDispatchWork)] () { > > I think you meant queueTaskKeepingThisObjectAlive ? Oops, yeah, that's a typo.

Ryosuke Niwa

Comment 13 2019-11-01 16:46:24 PDT

Created attachment 382652 [details] WIP6

Ryosuke Niwa

Comment 14 2019-11-01 16:50:41 PDT

Created attachment 382653 [details] WIP7

Ryosuke Niwa

Comment 15 2019-11-01 17:17:03 PDT

Created attachment 382660 [details] Patch

Chris Dumez

Comment 16 2019-11-02 10:13:10 PDT

Comment on attachment 382660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382660&action=review > Source/WebCore/dom/ActiveDOMObject.h:125 > + object.queueTaskInEventLoop(source, [protectedObject = makeRef(object), activity = object.makePendingActivity(object), movedTask = WTFMove(task)] () { Doesn't have to be in this patch but it feels like makePendingActivity() should be static too. movedTask can simply be named task. > Source/WebCore/dom/ActiveDOMObject.h:133 > + ASSERT(!event->target()); Why this assertion? > Source/WebCore/dom/ActiveDOMObject.h:134 > + queueTaskKeepingThisObjectAlive(target, source, [&target, movedEvent = WTFMove(event)] () mutable { How is capturing target by reference safe here? movedEvent can simply be called event. > Source/WebCore/dom/ActiveDOMObject.h:143 > + void queueTaskInEventLoop(TaskSource, WTF::Function<void ()>&&); WTF:: in unnecessary

Chris Dumez

Comment 17 2019-11-02 11:58:24 PDT

Comment on attachment 382660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382660&action=review Patch will need rebaselining. >> Source/WebCore/dom/ActiveDOMObject.h:134 >> + queueTaskKeepingThisObjectAlive(target, source, [&target, movedEvent = WTFMove(event)] () mutable { > > How is capturing target by reference safe here? > movedEvent can simply be called event. Oh, queueTaskKeepingThisObjectAlive() protected target so it is safe to capture it by reference indeed.

Ryosuke Niwa

Comment 18 2019-11-02 22:06:44 PDT

(In reply to Chris Dumez from comment #16) > Comment on attachment 382660 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382660&action=review > > > Source/WebCore/dom/ActiveDOMObject.h:125 > > + object.queueTaskInEventLoop(source, [protectedObject = makeRef(object), activity = object.makePendingActivity(object), movedTask = WTFMove(task)] () { > > Doesn't have to be in this patch but it feels like makePendingActivity() > should be static too. movedTask can simply be named task. > > > Source/WebCore/dom/ActiveDOMObject.h:133 > > + ASSERT(!event->target()); > > Why this assertion? This is to prevent the mistake of someone creating an event with a specific target that's different from "target" being passed to this function. > > Source/WebCore/dom/ActiveDOMObject.h:134 > > + queueTaskKeepingThisObjectAlive(target, source, [&target, movedEvent = WTFMove(event)] () mutable { > > How is capturing target by reference safe here? > movedEvent can simply be called event. It's because queueTaskKeepingThisObjectAlive keeps target alive as you described below. > > Source/WebCore/dom/ActiveDOMObject.h:143 > > + void queueTaskInEventLoop(TaskSource, WTF::Function<void ()>&&); > > WTF:: in unnecessary Removed.

Ryosuke Niwa

Comment 19 2019-11-02 22:08:12 PDT

I'd rename queueTaskKeepingThisObjectAlive to queueTaskKeepingObjectAlive now that it's a static function.

Chris Dumez

Comment 20 2019-11-02 23:12:09 PDT

Comment on attachment 382660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382660&action=review >> Source/WebCore/dom/ActiveDOMObject.h:133 >> + ASSERT(!event->target()); > > Why this assertion? I think this assertion should be tweaked to allow event.target() to be target. I do not see why the event target has to be null.

Ryosuke Niwa

Comment 21 2019-11-03 00:31:10 PDT

Created attachment 382697 [details] Patch for landing

Ryosuke Niwa

Comment 22 2019-11-03 00:31:42 PDT

Created attachment 382698 [details] Patch for landing

Ryosuke Niwa

Comment 23 2019-11-03 00:31:57 PDT

Comment on attachment 382698 [details] Patch for landing Wait for EWS once more.

Ryosuke Niwa

Comment 24 2019-11-03 00:33:41 PDT

(In reply to Chris Dumez from comment #20) > Comment on attachment 382660 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382660&action=review > > >> Source/WebCore/dom/ActiveDOMObject.h:133 > >> + ASSERT(!event->target()); > > > > Why this assertion? > > I think this assertion should be tweaked to allow event.target() to be > target. I do not see why the event target has to be null. Sure, did that.

WebKit Commit Bot

Comment 25 2019-11-03 12:02:03 PST

Comment on attachment 382698 [details] Patch for landing Clearing flags on attachment: 382698 Committed r251975: <https://trac.webkit.org/changeset/251975>

WebKit Commit Bot

Comment 26 2019-11-03 12:02:05 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 27 2019-11-03 12:03:18 PST

<rdar://problem/56850274>

Note You need to log in before you can comment on or make changes to this bug.