[webkit-reviews] review granted: [Bug 239066] Expose workers as service worker clients and implement registration matching for dedicated workers : [Attachment 457513] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 239066: Expose workers as service worker clients and implement registration
matching for dedicated workers
https://bugs.webkit.org/show_bug.cgi?id=239066

Attachment 457513: Patch

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




--- Comment #9 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 457513
  --> https://bugs.webkit.org/attachment.cgi?id=457513
Patch

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

r=me but I think there is a bug to fix before landing.

> Source/WebCore/workers/WorkerGlobalScope.cpp:662
> +    SWClientConnection& connection = swClientConnection();

Not sure why we need a local variable here.

> Source/WebCore/workers/WorkerScriptLoader.cpp:50
> +    static NeverDestroyed<HashMap<ScriptExecutionContextIdentifier,
WorkerScriptLoader*>> map;

Is this only used on the main thread? I assume so since I see no locking. If
so, we should use MainThreadNeverDestroyed.

> Source/WebCore/workers/service/SWClientConnection.cpp:244
> +	       updateController(*document, WTFMove(newController));

The WTFMove() here looks like a bug since we're in a for loop.

> Source/WebCore/workers/service/SWClientConnection.cpp:248
> +	       worker->postTaskToWorkerGlobalScope([newController =
WTFMove(newController).isolatedCopy()] (auto& context) mutable {

ditto.

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:72
> +    if (is<Document>(context)) {

if (auto* document = dynamicDowncast<Document>(context)) {