[webkit-reviews] review granted: [Bug 180669] Merge ServiceWorkerClientIdentifier into ServiceWorkerClientData : [Attachment 329031] 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 180669: Merge ServiceWorkerClientIdentifier into ServiceWorkerClientData
https://bugs.webkit.org/show_bug.cgi?id=180669

Attachment 329031: Patch

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




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

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

> Source/WebCore/dom/Document.cpp:7630
> +	   m_serviceWorkerConnection->unregisterServiceWorkerClient({
m_serviceWorkerConnection->serverConnectionIdentifier(), identifier() });

I wonder whether we should not have made SWClientConnection only take
DocumentIdentifier and do the translation to ServiceWorkerClientIdentifier on
its own.
Maybe this is too late for that. If not we could stick with passing a
DocumentIdentifier to unregisterServieWorkerClient.

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:65
> +    RELEASE_ASSERT(isDocument); // We do not support dedicated workers as
clients yet.

Isn't the downcast<Document> doing this release assert already?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:121
> +	       auto sourceClient =
ServiceWorkerClient::getOrCreate(serviceWorkerGlobalScope,
WTFMove(WTF::get<ServiceWorkerClientData>(sourceData)));

Maybe use RefPtr<ServiceWorkerClient> here instead of auto so that there is not
the need for it below for source =...?

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:93
> +    void unregisterServiceWorkerClient(const
WebCore::ServiceWorkerClientIdentifier&);

Sometimes we are using const ServiceWorkerClientIdentifier&, sometimes
ServiceWorkerClientIdentifier directly.