[webkit-reviews] review granted: [Bug 180383] Add support for ServiceWorkerContainer.prototype.ready : [Attachment 328472] 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 180383: Add support for ServiceWorkerContainer.prototype.ready
https://bugs.webkit.org/show_bug.cgi?id=180383

Attachment 328472: Patch

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




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

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

> Source/WebCore/workers/service/SWClientConnection.h:65
> +    using WhenRegistrationReadyCallback =
WTF::Function<void(ServiceWorkerRegistrationData&&)>;

If this cannot be a CompletionHandler, probably the other callbacks cannot be
either.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:93
> +			   return;

I wonder whether ScriptExecutionContext::postTaskTo would be able to prevent
these kind of checks in the future.

> Source/WebCore/workers/service/server/SWServer.cpp:650
> +	   connection->resolveRegistrationReadyRequests(registration);

Since we already expose Connection through getConnection, maybe it is fine to
allow iterating in some ways inside SWServerRegistration directly.

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:212
> +    m_registrationReadyRequests.append({ topOrigin, clientURL,
WTFMove(callback) });

If we want to keep good WK2/WebCore layering, we should probably add this
vector somewhere else, maybe at SWServerConnection level.
WebSWServerConnection::resolveRegistrationReadyRequests would also move up
there.

In preexisting code, we should probably add an
SWServerConnection::matchRegistration that would return an optional so that we
would do less WebCore stuff in WebSWServerConnection::matchRegistration.
The same could apply there as well maybe.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:158
> +void WebSWClientConnection::whenRegistrationReady(Ref<SecurityOrigin>&&
topOrigin, const URL& clientURL, WhenRegistrationReadyCallback&& callback)

Could probably take a const SecurityOrigin&.