[webkit-reviews] review granted: [Bug 180328] Support serviceWorker.postMessage() inside service workers : [Attachment 328299] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 180328: Support serviceWorker.postMessage() inside service workers
https://bugs.webkit.org/show_bug.cgi?id=180328

Attachment 328299: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 328299
  --> https://bugs.webkit.org/attachment.cgi?id=328299
Patch

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

> Source/WebCore/workers/service/SWClientConnection.h:75
> +    virtual void postMessageToServiceWorker(ServiceWorkerIdentifier
destinationIdentifier, Ref<SerializedScriptValue>&&,
ServiceWorkerClientIdentifier sourceIdentifier, ServiceWorkerClientData&&
source) = 0;
> +    virtual void postMessageToServiceWorker(ServiceWorkerIdentifier
destinationIdentifier, Ref<SerializedScriptValue>&&, ServiceWorkerIdentifier
sourceWorkerIdentifier) = 0;

I don’t think we need the names "destinationIdentifier", "sourceIdentifier", or
"source". Types make things clear enough. I might have named the
Ref<SerializedScriptValue>&& "message", but I suppose we can do without that
too.

But maybe I am wrong and it’s not clear enough without them; the types are
different but maybe it’s too subtle. Not sure.

> Source/WebCore/workers/service/ServiceWorker.cpp:117
> +	       auto& swConnection =
ServiceWorkerProvider::singleton().serviceWorkerConnectionForSession(sessionID)
;

How about just "connection" for this variable? We are in a class named
ServiceWorker here.

> Source/WebCore/workers/service/ServiceWorker.cpp:124
>      auto& swConnection =
ServiceWorkerProvider::singleton().serviceWorkerConnectionForSession(context.se
ssionID());

Ditto.

> Source/WebCore/workers/service/context/SWContextManager.h:70
> +    WEBCORE_EXPORT void postMessageToServiceWorker(ServiceWorkerIdentifier
destination, Ref<SerializedScriptValue>&& message,
ServiceWorkerClientIdentifier sourceIdentifier, ServiceWorkerClientData&&
sourceData);
> +    WEBCORE_EXPORT void postMessageToServiceWorker(ServiceWorkerIdentifier
destination, Ref<SerializedScriptValue>&& message, ServiceWorkerData&&
sourceData);

I don’t think we need the names "destination", "sourceIdentifier" and
"sourceData". Same thinking as above.

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:108
> +static void fireMessageEvent(ServiceWorkerGlobalScope&
serviceWorkerGlobalScope, Ref<SerializedScriptValue>&& message,
std::unique_ptr<MessagePortChannelArray>&& channels,
ExtendableMessageEventSource&& source, Ref<SecurityOrigin>&& sourceOrigin)

Can this just be named "scope" or "globalScope"  instead of
"serviceWorkerGlobalScope"?

> Source/WebCore/workers/service/context/ServiceWorkerThread.h:62
> +    WEBCORE_EXPORT void
postMessageToServiceWorker(Ref<SerializedScriptValue>&&,
std::unique_ptr<MessagePortChannelArray>&&, ServiceWorkerClientIdentifier
sourceIdentifier, ServiceWorkerClientData&& sourceData);
> +    WEBCORE_EXPORT void
postMessageToServiceWorker(Ref<SerializedScriptValue>&&,
std::unique_ptr<MessagePortChannelArray>&&, ServiceWorkerData&& sourceData);

Same thoughts about argument names as above.

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:84
> +    void
postMessageToServiceWorkerFromClient(WebCore::ServiceWorkerIdentifier
destinationIdentifier, const IPC::DataReference& message,
WebCore::ServiceWorkerClientIdentifier sourceIdentifier,
WebCore::ServiceWorkerClientData&& source);
> +    void
postMessageToServiceWorkerFromServiceWorker(WebCore::ServiceWorkerIdentifier
destinationIdentifier, const IPC::DataReference& message,
WebCore::ServiceWorkerIdentifier sourceIdentifier);

And again.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:77
> +    void postMessageToServiceWorker(WebCore::ServiceWorkerIdentifier
destinationIdentifier, Ref<WebCore::SerializedScriptValue>&&,
WebCore::ServiceWorkerClientIdentifier sourceIdentifier,
WebCore::ServiceWorkerClientData&& source) final;
> +    void postMessageToServiceWorker(WebCore::ServiceWorkerIdentifier
destinationIdentifier, Ref<WebCore::SerializedScriptValue>&&,
WebCore::ServiceWorkerIdentifier sourceWorkerIdentifier) final;

And again.

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:76
> +    void
postMessageToServiceWorkerFromClient(WebCore::ServiceWorkerIdentifier
destinationIdentifier, const IPC::DataReference& message,
WebCore::ServiceWorkerClientIdentifier sourceIdentifier,
WebCore::ServiceWorkerClientData&& sourceData);
> +    void
postMessageToServiceWorkerFromServiceWorker(WebCore::ServiceWorkerIdentifier
destinationIdentifier, const IPC::DataReference& message,
WebCore::ServiceWorkerData&& sourceData);

And again.