[webkit-reviews] review granted: [Bug 179822] [Service Workers] Implement "Notify Controller Change" algorithm : [Attachment 327194] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 179822: [Service Workers] Implement "Notify Controller Change" algorithm
https://bugs.webkit.org/show_bug.cgi?id=179822

Attachment 327194: Patch

https://bugs.webkit.org/attachment.cgi?id=327194&action=review




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 327194
  --> https://bugs.webkit.org/attachment.cgi?id=327194
Patch

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:419
> +    scriptExecutionContext()->postTask([this, protectedThis =
makeRef(*this)](ScriptExecutionContext&) mutable {

I know the spec is written this way but It seems a bit weird to post a task
here for dispatching an event.
It does not seem observable to dispatch the event directly without the post
task, at least in the current context it is used.
Might be safer to stick to the spec though, not sure...

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:424
> +	   dispatchEvent(controllerChangeEvent);

do we need controllerChangeEvent variable?

> Source/WebCore/workers/service/server/SWClientConnection.cpp:168
> +void SWClientConnection::notifyClientsOfControllerChange(const
HashSet<uint64_t>& scriptExecutionContexts, const ServiceWorkerData&
newController)

newController can probably be an r value.

> Source/WebCore/workers/service/server/SWClientConnection.cpp:177
> +	   client->setActiveServiceWorker(ServiceWorker::getOrCreate(*client,
ServiceWorkerData { newController }));

Maybe we should assert that client has an active service worker.
Not sure whether we could also assert that we are actually changing from
worker.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:235
> +    // - Set clientâs active worker to registrationâs active worker.

I see client^as here.

> LayoutTests/http/tests/workers/service/controller-change.html:38
> +    let frame = await withFrame(scopeURL);

In sw-test-pre.js, there is an interceptedFrame function that could be reused
to replace register and withFrame.