Bug 180669 – Merge ServiceWorkerClientIdentifier into ServiceWorkerClientData

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Merge ServiceWorkerClientIdentifier into ServiceWorkerCientData, for consistency with ServiceWorkerIdentifier / ServiceWorkerData, and start simplifying the postMessage() code.

[Chris Dumez]

Created attachment 329030 [details]
Patch

[Chris Dumez]

Created attachment 329031 [details]
Patch

[youenn fablet]

Comment on attachment 329031 [details]
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.

[Chris Dumez]

Comment on attachment 329031 [details]
Patch

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

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

Nah, only a ASSERT_WITH_SECURITY_IMPLICATION() (i.e. debug only)

[Chris Dumez]

Created attachment 329042 [details]
Patch

[Chris Dumez]

Comment on attachment 329042 [details]
Patch

Clearing flags on attachment: 329042

Committed r225760: <https://trac.webkit.org/changeset/225760>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35980151>