Bug 203714 – Add a helper function to schedule a task to dispatch an event to ActiveDOMObject

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 382643 [details]
WIP

[Ryosuke Niwa]

Created attachment 382645 [details]
WIP2

[Ryosuke Niwa]

Created attachment 382646 [details]
WIP3

[Chris Dumez]

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

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

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

[Ryosuke Niwa]

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

Created attachment 382650 [details]
WIP4

[Chris Dumez]

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]

Created attachment 382651 [details]
WIP5

[Ryosuke Niwa]

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

Created attachment 382652 [details]
WIP6

[Ryosuke Niwa]

Created attachment 382653 [details]
WIP7

[Ryosuke Niwa]

Created attachment 382660 [details]
Patch

[Chris Dumez]

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

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

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

[Chris Dumez]

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]

Created attachment 382697 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 382698 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 382698 [details]
Patch for landing

Wait for EWS once more.

[Ryosuke Niwa]

(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 on attachment 382698 [details]
Patch for landing

Clearing flags on attachment: 382698

Committed r251975: <https://trac.webkit.org/changeset/251975>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56850274>