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: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 203680 | ||||||||||||||||||||||||
Bug Blocks: | 203667 | ||||||||||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2019-10-31 19:17:06 PDT
Created attachment 382643 [details]
WIP
Created attachment 382645 [details]
WIP2
Created attachment 382646 [details]
WIP3
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 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 :) (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. And BTW, I think those are going to be super helpful. (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. Created attachment 382650 [details]
WIP4
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 ? Created attachment 382651 [details]
WIP5
(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. Created attachment 382652 [details]
WIP6
Created attachment 382653 [details]
WIP7
Created attachment 382660 [details]
Patch
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 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. (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. I'd rename queueTaskKeepingThisObjectAlive to queueTaskKeepingObjectAlive now that it's a static function. 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. Created attachment 382697 [details]
Patch for landing
Created attachment 382698 [details]
Patch for landing
Comment on attachment 382698 [details]
Patch for landing
Wait for EWS once more.
(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 on attachment 382698 [details] Patch for landing Clearing flags on attachment: 382698 Committed r251975: <https://trac.webkit.org/changeset/251975> All reviewed patches have been landed. Closing bug. |