See https://github.com/whatwg/html/issues/512
Created attachment 346275 [details] Patch
Comment on attachment 346275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346275&action=review > Source/WebCore/workers/WorkerGlobalScope.cpp:256 > + postTask([callback = WTFMove(callback)](ScriptExecutionContext&) { How can this be correct? Microtasks & tasks are completely different concepts & timing.
Spec: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#microtask-queuing Tests: https://github.com/web-platform-tests/wpt/pull/6334
Comment on attachment 346275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346275&action=review >> Source/WebCore/workers/WorkerGlobalScope.cpp:256 >> + postTask([callback = WTFMove(callback)](ScriptExecutionContext&) { > > How can this be correct? Microtasks & tasks are completely different concepts & timing. I think we need to hook into JSC's microtask queue instead here.
Comment on attachment 346275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346275&action=review >>> Source/WebCore/workers/WorkerGlobalScope.cpp:256 >>> + postTask([callback = WTFMove(callback)](ScriptExecutionContext&) { >> >> How can this be correct? Microtasks & tasks are completely different concepts & timing. > > I think we need to hook into JSC's microtask queue instead here. "How can this be correct? Microtasks & tasks are completely different concepts & timing." Yes, this is correct in terms of our current implementation. Unfortunately, our Worker does not support Microtasks as our main thread does. So this code is the same to the current Worker's microtask queuing mechanism. We can find that JSWorkerGlobalScopeBase::queueTaskToEventLoop does the same thing, while JSDOMWindowBase::queueTaskToEventLoop posts microtasks correctly. "I think we need to hook into JSC's microtask queue instead here." The current approach against using JSC's microtask queue has Pros and Cons. Pros: - It skips one level indirection (JSC -> WebCore through function pointer) - It leverages binding VoidCallback type checking instead of manually typechecking in custom binding code Cons: - It does not use JSC's microtask stack, consistency Given that this `self.queueMicrotask` is performance-sensitive (If it is not performance-sensitive, we can just use `Promise.resolve().then(callback)`), and I prefer non-custom binding code, I took the current approach in this patch. But if the consistency is preferable, we can use JSC's stack in binding custom code. I'll upload this version.
Here's a problem though. If the worker ever supports a feature which schedules a task, then all microtasks are supposed to be drained before such a task is executed. The proposed patch doesn't implement this semantics. I don't think we want to be in the business of auditing everything worker supports and making sure they never schedules a task.
(In reply to Ryosuke Niwa from comment #6) > Here's a problem though. > > If the worker ever supports a feature which schedules a task, then all > microtasks are supposed to be drained before such a task is executed. The > proposed patch doesn't implement this semantics. > > I don't think we want to be in the business of auditing everything worker > supports and making sure they never schedules a task. OK, then, I'll implement only DOM window version for now.
Created attachment 346353 [details] Patch
Created attachment 346354 [details] Patch
Created attachment 346355 [details] Patch
Comment on attachment 346355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review > Source/JavaScriptCore/runtime/JSJob.cpp:-48 > - virtual ~JSJobMicrotask() Where did the destructor go? > Source/WebCore/page/DOMWindow.idl:92 > + [Custom] void queueMicrotask(VoidCallback callback); So we're not gonna implement the one in worker??
Comment on attachment 346355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review > Source/JavaScriptCore/runtime/JSJob.cpp:60 > +Ref<Microtask> createJSJob(VM& vm, JSValue job) I think we should call this createJSTask instead to use the consistent terminology.
Comment on attachment 346355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review >> Source/JavaScriptCore/runtime/JSJob.cpp:-48 >> - virtual ~JSJobMicrotask() > > Where did the destructor go? Microtask has `virtual ~Microtask()`, so this is not necessary. >> Source/WebCore/page/DOMWindow.idl:92 >> + [Custom] void queueMicrotask(VoidCallback callback); > > So we're not gonna implement the one in worker?? Yes, we will implement it after MicrotaskQueue is adopted in worker environments.
Comment on attachment 346355 [details] Patch Sounds good to me.
Let's file a bug for the worker one so that we don't forget though.
Comment on attachment 346355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review >> Source/JavaScriptCore/runtime/JSJob.cpp:60 >> +Ref<Microtask> createJSJob(VM& vm, JSValue job) > > I think we should call this createJSTask instead to use the consistent terminology. The interesting thing is that this "Job" is a word used in ECMA262 :P https://tc39.github.io/ecma262/#sec-enqueuejob Using a word "microtask" in WebKit tree consistently sounds fine. Fixed. >>> Source/WebCore/page/DOMWindow.idl:92 >>> + [Custom] void queueMicrotask(VoidCallback callback); >> >> So we're not gonna implement the one in worker?? > > Yes, we will implement it after MicrotaskQueue is adopted in worker environments. The interesting thing is that this "Job" is a word used in ECMA262 :P https://tc39.github.io/ecma262/#sec-enqueuejob Using a word "microtask" in WebKit tree consistently sounds fine. Fixed.
(In reply to Ryosuke Niwa from comment #15) > Let's file a bug for the worker one so that we don't forget though. OK, filed in bug 188246 and bug 188247
Oh, you're right: https://html.spec.whatwg.org/#integration-with-the-javascript-job-queue But I think the definition of job there is slightly different from what we're doing here.
Committed r234491: <https://trac.webkit.org/changeset/234491>
<rdar://problem/42841210>