Bug 180328

Summary: Support serviceWorker.postMessage() inside service workers
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, darin, ggaren, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 180329    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-12-02 21:46:33 PST
Support serviceWorker.postMessage() inside service workers.
Comment 1 Chris Dumez 2017-12-02 21:55:39 PST
Created attachment 328282 [details]
WIP Patch
Comment 2 Chris Dumez 2017-12-02 22:02:55 PST
Created attachment 328283 [details]
Patch
Comment 3 Chris Dumez 2017-12-03 13:01:51 PST
Created attachment 328299 [details]
Patch
Comment 4 Darin Adler 2017-12-03 13:24:26 PST
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.
Comment 5 Chris Dumez 2017-12-03 14:22:19 PST
Created attachment 328306 [details]
Patch
Comment 6 Chris Dumez 2017-12-03 14:23:48 PST
(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.
Comment 7 Chris Dumez 2017-12-03 14:24:50 PST
Comment on attachment 328306 [details]
Patch

Clearing flags on attachment: 328306

Committed r225462: <https://trac.webkit.org/changeset/225462>
Comment 8 Chris Dumez 2017-12-03 14:24:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-12-03 14:25:24 PST
<rdar://problem/35820398>