[webkit-reviews] review granted: [Bug 180360] Support container.getRegistration() / getRegistrations() inside service workers : [Attachment 328397] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 4 16:16:08 PST 2017

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

Attachment 328397: Patch


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

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
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

> 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?

More information about the webkit-reviews mailing list