Support serviceWorker.postMessage() inside service workers

[Chris Dumez]

Support serviceWorker.postMessage() inside service workers.

[Chris Dumez]

Created attachment 328282 [details]
WIP Patch

[Chris Dumez]

Created attachment 328283 [details]
Patch

[Chris Dumez]

Created attachment 328299 [details]
Patch

[Darin Adler]

Comment on attachment 328299 [details]
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.sessionID());

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.

[Chris Dumez]

Created attachment 328306 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 328299 [details]
> 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.sessionID());
> 
> 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.

I feel that distinguishing the destination and source parameters is important. It would be especially confusing for messages from a ServiceWorker to another ServiceWorker.

[Chris Dumez]

Comment on attachment 328306 [details]
Patch

Clearing flags on attachment: 328306

Committed r225462: <https://trac.webkit.org/changeset/225462>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35820398>