Bug 180328

Created attachment 328282 [details] WIP Patch

Created attachment 328283 [details] Patch

Created attachment 328299 [details] Patch

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.

Created attachment 328306 [details] Patch

(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 on attachment 328306 [details] Patch Clearing flags on attachment: 328306 Committed r225462: <https://trac.webkit.org/changeset/225462>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35820398>