[webkit-reviews] review granted: [Bug 180360] Support container.getRegistration() / getRegistrations() inside service workers : [Attachment 328397] 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 180360: Support container.getRegistration() / getRegistrations() inside
service workers
https://bugs.webkit.org/show_bug.cgi?id=180360

Attachment 328397: Patch

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




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

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

> Source/WebCore/dom/ScriptExecutionContext.cpp:588
> +void ScriptExecutionContext::postTaskTo(const DocumentOrWorkerIdentifier&
contextIdentifier, WTF::Function<void(ScriptExecutionContext&)>&& task)

I am always wondering whether to use Identifier or const Identifier& in such
cases.
I have difficulties keeping it consistent. Since it is a Variant, it is better
to pass it by ref?

> Source/WebCore/dom/ScriptExecutionContext.cpp:592
> +    switchOn(contextIdentifier, [&](DocumentIdentifier identifier) {

space between ] and ( is apparently our style.

> Source/WebCore/dom/ScriptExecutionContext.cpp:596
> +	   document->postTask([task = WTFMove(task)](ScriptExecutionContext&
scope) {

auto& here and in other various places below ?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:232
> +    Ref<SecurityOrigin> topOrigin = context->topOrigin();

This may end up topOrigin to be destroyed on the main thread. Shouldn't we
isolate it?
Ditto below probably.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:320
> +    }

You can probably write it as:
auto registrations = WTF::map(WTFMove(registrations), [this] (auto&&
registrationData) {
    return ServiceWorkerRegistration::getOrCreate(*scriptExecutionContext(),
*this, WTFMove(registrationData));
});

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:545
> +    m_pendingGetRegistrationsRequests.clear();

I would try to use one map that keeps DeferredPromise instead of typed
DOMPromiseDeferred.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:140
> +    ASSERT(isMainThread());

We do not do that often when receiving IPC messages.
Should we try to do that more aggressively? Or should these assertions be moved
to the lambdas instead?