Bug 203714

Summary: Add a helper function to schedule a task to dispatch an event to ActiveDOMObject
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: 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: P2 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

Description Ryosuke Niwa 2019-10-31 19:17:06 PDT
Refactor a helper function added in https://bugs.webkit.org/show_bug.cgi?id=203680 to ActiveDOMObject.
Comment 1 Ryosuke Niwa 2019-11-01 16:05:01 PDT
Created attachment 382643 [details]
WIP
Comment 2 Ryosuke Niwa 2019-11-01 16:08:03 PDT
Created attachment 382645 [details]
WIP2
Comment 3 Ryosuke Niwa 2019-11-01 16:13:46 PDT
Created attachment 382646 [details]
WIP3
Comment 4 Chris Dumez 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.
Comment 5 Chris Dumez 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 :)
Comment 6 Ryosuke Niwa 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.
Comment 7 Chris Dumez 2019-11-01 16:33:52 PDT
And BTW, I think those are going to be super helpful.
Comment 8 Ryosuke Niwa 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.
Comment 9 Ryosuke Niwa 2019-11-01 16:39:01 PDT
Created attachment 382650 [details]
WIP4
Comment 10 Chris Dumez 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 ?
Comment 11 Ryosuke Niwa 2019-11-01 16:44:11 PDT
Created attachment 382651 [details]
WIP5
Comment 12 Ryosuke Niwa 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.
Comment 13 Ryosuke Niwa 2019-11-01 16:46:24 PDT
Created attachment 382652 [details]
WIP6
Comment 14 Ryosuke Niwa 2019-11-01 16:50:41 PDT
Created attachment 382653 [details]
WIP7
Comment 15 Ryosuke Niwa 2019-11-01 17:17:03 PDT
Created attachment 382660 [details]
Patch
Comment 16 Chris Dumez 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
Comment 17 Chris Dumez 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.
Comment 18 Ryosuke Niwa 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.
Comment 19 Ryosuke Niwa 2019-11-02 22:08:12 PDT
I'd rename queueTaskKeepingThisObjectAlive to queueTaskKeepingObjectAlive now that it's a static function.
Comment 20 Chris Dumez 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.
Comment 21 Ryosuke Niwa 2019-11-03 00:31:10 PDT
Created attachment 382697 [details]
Patch for landing
Comment 22 Ryosuke Niwa 2019-11-03 00:31:42 PDT
Created attachment 382698 [details]
Patch for landing
Comment 23 Ryosuke Niwa 2019-11-03 00:31:57 PDT
Comment on attachment 382698 [details]
Patch for landing

Wait for EWS once more.
Comment 24 Ryosuke Niwa 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.
Comment 25 WebKit Commit Bot 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>
Comment 26 WebKit Commit Bot 2019-11-03 12:02:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2019-11-03 12:03:18 PST
<rdar://problem/56850274>