[webkit-reviews] review granted: [Bug 183873] Promptly terminate service worker processes when they are no longer needed : [Attachment 336384] 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 183873: Promptly terminate service worker processes when they are no longer
needed
https://bugs.webkit.org/show_bug.cgi?id=183873

Attachment 336384: Patch

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




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

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

> Source/WebCore/workers/service/server/SWServer.cpp:745
> +    auto& clientIdentifiersForSecurityOrigin =
m_clientsBySecurityOrigin.ensure(WTFMove(securityOrigin), [] {

Would it be more efficient and simpler if m_clientsBySecurityOrigin takes a
SecurityOriginData?
It is unfortunate that we would need to create all these Ref<SecurityOrigin>
just for querying this map.
I guess there are other places that would be affected by this change.

> Source/WebCore/workers/service/server/SWServer.cpp:789
> +	   if (m_shouldDisableServiceWorkerProcessTerminationDelay)

It might be better to keep the idea of a timer for terminating workers even in
the context of API tests.
This would make it closer to actual WebKit behavior.
Can we instead do
startOneShort(m_shouldDisableServiceWorkerProcessTerminationDelay ? 0 :
terminationDelay); ?

Or maybe, since we have this piping from UIProcess to WebProcess, we could pass
the timer value itself in case we want at some point to give application the
ability to set this value.

> Source/WebCore/workers/service/server/SWServer.cpp:801
> +    clientsForSecurityOrigin.removeFirstMatching([&] (const auto&
identifier) {

Would it be better if it was a HashSet. We probably do not need to keep the
order here.
Maybe we can also assert that we are actually finding this clientIdentifier.

> Source/WebKit/StorageProcess/StorageProcess.cpp:134
> +    return false;

I wonder whether WTF::anyOf(m_swServers.values(), [&]...) would work here.