Bug 203919

Summary: Port Worker to the HTML5 event loop
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, esprehn+autocc, ews-watchlist, ggaren, kangil.han, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 202843    
Attachments:
Description Flags
Patch rniwa: review+

Description Chris Dumez 2019-11-06 14:16:21 PST
Port Worker to the HTML5 event loop.
Comment 1 Chris Dumez 2019-11-06 19:29:31 PST
Created attachment 383014 [details]
Patch
Comment 2 Ryosuke Niwa 2019-11-07 14:00:17 PST
Comment on attachment 383014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383014&action=review

> Source/WebCore/workers/Worker.cpp:208
> +        queueTaskToDispatchEvent(*this, TaskSource::DOMManipulation, Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::Yes));

Shouldn't this be Networking?

> Source/WebCore/workers/WorkerMessagingProxy.cpp:107
> -        workerObject->enqueueEvent(MessageEvent::create(WTFMove(ports), message.message.releaseNonNull()));
> +        ActiveDOMObject::queueTaskToDispatchEvent(*workerObject, TaskSource::DOMManipulation, MessageEvent::create(WTFMove(ports), message.message.releaseNonNull()));

Shouldn't this be unshipped port message queue?
https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps

> Source/WebCore/workers/WorkerMessagingProxy.cpp:180
> -        workerObject->enqueueEvent(ErrorEvent::create(errorMessage, sourceURL, lineNumber, columnNumber, { }));
> +        ActiveDOMObject::queueTaskToDispatchEvent(*workerObject, TaskSource::DOMManipulation, ErrorEvent::create(errorMessage, sourceURL, lineNumber, columnNumber, { }));

Ditto.
Comment 3 Chris Dumez 2019-11-07 14:03:11 PST
Comment on attachment 383014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383014&action=review

>> Source/WebCore/workers/Worker.cpp:208
>> +        queueTaskToDispatchEvent(*this, TaskSource::DOMManipulation, Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::Yes));
> 
> Shouldn't this be Networking?

The spec says:
"The task source for the tasks mentioned above is the DOM manipulation task source."
Comment 4 Chris Dumez 2019-11-07 14:04:36 PST
Comment on attachment 383014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383014&action=review

>> Source/WebCore/workers/WorkerMessagingProxy.cpp:107
>> +        ActiveDOMObject::queueTaskToDispatchEvent(*workerObject, TaskSource::DOMManipulation, MessageEvent::create(WTFMove(ports), message.message.releaseNonNull()));
> 
> Shouldn't this be unshipped port message queue?
> https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps

We don't exactly match the spec here indeed but I am unclear how to exactly match the spec.
Would you like me to add a FIXME comment?
Comment 5 Ryosuke Niwa 2019-11-07 14:07:50 PST
(In reply to Chris Dumez from comment #4)
> Comment on attachment 383014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383014&action=review
> 
> >> Source/WebCore/workers/WorkerMessagingProxy.cpp:107
> >> +        ActiveDOMObject::queueTaskToDispatchEvent(*workerObject, TaskSource::DOMManipulation, MessageEvent::create(WTFMove(ports), message.message.releaseNonNull()));
> > 
> > Shouldn't this be unshipped port message queue?
> > https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps
> 
> We don't exactly match the spec here indeed but I am unclear how to exactly
> match the spec.
> Would you like me to add a FIXME comment?

I think using DOM manipulation task source is super confusing here because it clearly isn't that. I think the closest thing is https://html.spec.whatwg.org/multipage/web-messaging.html#port-message-queue

See https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps, which is invoked by postMessage on Worker, which uses the implicitly defined MessagePort of Worker.
Comment 6 Chris Dumez 2019-11-07 14:10:16 PST
Comment on attachment 383014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383014&action=review

>>>> Source/WebCore/workers/WorkerMessagingProxy.cpp:107
>>>> +        ActiveDOMObject::queueTaskToDispatchEvent(*workerObject, TaskSource::DOMManipulation, MessageEvent::create(WTFMove(ports), message.message.releaseNonNull()));
>>> 
>>> Shouldn't this be unshipped port message queue?
>>> https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps
>> 
>> We don't exactly match the spec here indeed but I am unclear how to exactly match the spec.
>> Would you like me to add a FIXME comment?
> 
> I think using DOM manipulation task source is super confusing here because it clearly isn't that. I think the closest thing is https://html.spec.whatwg.org/multipage/web-messaging.html#port-message-queue
> 
> See https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps, which is invoked by postMessage on Worker, which uses the implicitly defined MessagePort of Worker.

I used DOMManipulation for consistency with the rest of the spec. Yes, the spec is using DOMManipulation for things that are not DOMManipulation. See also:
https://w3c.github.io/ServiceWorker/#dom-serviceworker-postmessage-message-options

Even in service workers, the spec says to use DOMManipulation for postMessage.
Comment 7 Ryosuke Niwa 2019-11-07 14:17:43 PST
(In reply to Chris Dumez from comment #6)
> Comment on attachment 383014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383014&action=review
> 
> >>>> Source/WebCore/workers/WorkerMessagingProxy.cpp:107
> >>>> +        ActiveDOMObject::queueTaskToDispatchEvent(*workerObject, TaskSource::DOMManipulation, MessageEvent::create(WTFMove(ports), message.message.releaseNonNull()));
> >>> 
> >>> Shouldn't this be unshipped port message queue?
> >>> https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps
> >> 
> >> We don't exactly match the spec here indeed but I am unclear how to exactly match the spec.
> >> Would you like me to add a FIXME comment?
> > 
> > I think using DOM manipulation task source is super confusing here because it clearly isn't that. I think the closest thing is https://html.spec.whatwg.org/multipage/web-messaging.html#port-message-queue
> > 
> > See https://html.spec.whatwg.org/multipage/web-messaging.html#message-port-post-message-steps, which is invoked by postMessage on Worker, which uses the implicitly defined MessagePort of Worker.
> 
> I used DOMManipulation for consistency with the rest of the spec. Yes, the
> spec is using DOMManipulation for things that are not DOMManipulation. See
> also:
> https://w3c.github.io/ServiceWorker/#dom-serviceworker-postmessage-message-
> options
> 
> Even in service workers, the spec says to use DOMManipulation for
> postMessage.

That's very strange. Perhaps we need a spec bug to clarify what task source is used for messaging. I'd much prefer having a single task source for all messaging related APIs than ramp it together with DOM manipulation.
Comment 8 Chris Dumez 2019-11-07 15:24:46 PST
Committed r252212: <https://trac.webkit.org/changeset/252212>
Comment 9 Radar WebKit Bug Importer 2019-11-07 15:25:19 PST
<rdar://problem/57000977>