Add initial support for serviceWorkerClient.postMessage(): - https://w3c.github.io/ServiceWorker/#client-postmessage
Created attachment 324830 [details] WIP Patch
Created attachment 324833 [details] WIP Patch
Attachment 324833 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 324889 [details] WIP Patch
Created attachment 324899 [details] WIP Patch
Attachment 324899 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/ServiceWorkerClient.h:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 324917 [details] WIP Patch
Attachment 324917 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 324926 [details] WIP Patch
Attachment 324926 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 324929 [details] WIP Patch Ok. Will work on cleaning this up but communication works in both directions now.
Attachment 324929 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 324947 [details] WIP Patch
Attachment 324947 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 325017 [details] WIP Patch
Created attachment 325019 [details] WIP Patch
Attachment 325019 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 325077 [details] WIP Patch
Attachment 325077 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:444: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 325097 [details] WIP Patch
Created attachment 325099 [details] Patch
Comment on attachment 325099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325099&action=review > Source/WebCore/ChangeLog:12 > + via postMessage(). Cool! > Source/WebCore/dom/Document.cpp:445 > + static NeverDestroyed<HashMap<Identifier, Document*>> documents; We should be able to do HashMap<Identifier, reference_wrapper> at some point. > Source/WebCore/dom/Document.h:350 > + using Identifier = uint64_t; Does Identifier really help? If it helps, should we try to ensure that no implicit casting happens between uint64_t and Identifier? > Source/WebCore/workers/service/ServiceWorkerClient.cpp:71 > + Vector<RefPtr<MessagePort>> ports; Might want Vector<Ref> at some point. > Source/WebCore/workers/service/ServiceWorkerClient.cpp:87 > + callOnMainThread([message = message.releaseReturnValue(), destinationIdentifier = m_identifier, sourceIdentifier, sourceOrigin = context.origin().isolatedCopy()] () mutable { Probably fine or even better using callOnMainThread here. I think the expected abstraction is postMessageToWorkerObject though. Maybe we should do some refactoring. > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:34 > + uint64_t scriptExecutionContextIdentifier; We really have a process id and a per-process environment identifier. It might be good to have something more generic, so that we can reuse this environment identifier in other places like network loads too. This can probably be improved later on when we have more concrete plans. > Source/WebCore/workers/service/context/SWContextManager.h:47 > + virtual void postMessageToServiceWorkerClient(const ServiceWorkerClientIdentifier& destinationIdentifier, Ref<SerializedScriptValue>&& message, uint64_t sourceServiceWorkerIdentifier, const String& sourceOrigin) = 0; No need for destinationIdentifier name. > Source/WebCore/workers/service/server/SWClientConnection.cpp:123 > + if (!container) Can this be a real regular case? > Source/WebCore/workers/service/server/SWClientConnection.cpp:131 > + source = MessageEventSource { RefPtr<ServiceWorker> { ServiceWorker::create(*destinationDocument, sourceServiceWorkerIdentifier) } }; Every time we receive a message, we will create a new ServiceWorker. We should reuse them according their ids and if we create one, it should be based on a full ServiceWorkerRegistration. Can we fix that now? > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906 > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier) The current style is to move this code closer to the definition, i.e. ServiceWorkerClientIdentifier.
Created attachment 325166 [details] Patch
(In reply to youenn fablet from comment #22) > Comment on attachment 325099 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325099&action=review > > > Source/WebCore/ChangeLog:12 > > + via postMessage(). > > Cool! > > > Source/WebCore/dom/Document.cpp:445 > > + static NeverDestroyed<HashMap<Identifier, Document*>> documents; > > We should be able to do HashMap<Identifier, reference_wrapper> at some point. > > > Source/WebCore/dom/Document.h:350 > > + using Identifier = uint64_t; > > Does Identifier really help? > If it helps, should we try to ensure that no implicit casting happens > between uint64_t and Identifier? Ok, will drop Identifier alias. > > Source/WebCore/workers/service/ServiceWorkerClient.cpp:71 > > + Vector<RefPtr<MessagePort>> ports; > > Might want Vector<Ref> at some point. > > > Source/WebCore/workers/service/ServiceWorkerClient.cpp:87 > > + callOnMainThread([message = message.releaseReturnValue(), destinationIdentifier = m_identifier, sourceIdentifier, sourceOrigin = context.origin().isolatedCopy()] () mutable { > > Probably fine or even better using callOnMainThread here. > I think the expected abstraction is postMessageToWorkerObject though. > Maybe we should do some refactoring. > > > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:34 > > + uint64_t scriptExecutionContextIdentifier; > > We really have a process id and a per-process environment identifier. > It might be good to have something more generic, so that we can reuse this > environment identifier in other places like network loads too. > This can probably be improved later on when we have more concrete plans. > > > Source/WebCore/workers/service/context/SWContextManager.h:47 > > + virtual void postMessageToServiceWorkerClient(const ServiceWorkerClientIdentifier& destinationIdentifier, Ref<SerializedScriptValue>&& message, uint64_t sourceServiceWorkerIdentifier, const String& sourceOrigin) = 0; > > No need for destinationIdentifier name. > > > Source/WebCore/workers/service/server/SWClientConnection.cpp:123 > > + if (!container) > > Can this be a real regular case? > > > Source/WebCore/workers/service/server/SWClientConnection.cpp:131 > > + source = MessageEventSource { RefPtr<ServiceWorker> { ServiceWorker::create(*destinationDocument, sourceServiceWorkerIdentifier) } }; > > Every time we receive a message, we will create a new ServiceWorker. > We should reuse them according their ids and if we create one, it should be > based on a full ServiceWorkerRegistration. > Can we fix that now? This is not an issue yet because we currently always reuse the controller. At some point, it'd be good to keep a list of ServiceWorkers somewhere I guess. > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906 > > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier) > > The current style is to move this code closer to the definition, i.e. > ServiceWorkerClientIdentifier. Can you please clarify this?
Comment on attachment 325166 [details] Patch Clearing flags on attachment: 325166 Committed r224113: <https://trac.webkit.org/changeset/224113>
All reviewed patches have been landed. Closing bug.
> > Every time we receive a message, we will create a new ServiceWorker. > > We should reuse them according their ids and if we create one, it should be > > based on a full ServiceWorkerRegistration. > > Can we fix that now? > > This is not an issue yet because we currently always reuse the controller. > At some point, it'd be good to keep a list of ServiceWorkers somewhere I > guess. Maybe we should add A FIXME and not implement this code path then. > > > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906 > > > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier) > > > > The current style is to move this code closer to the definition, i.e. > > ServiceWorkerClientIdentifier. > > Can you please clarify this? Add the encoder/decoder in ServiceWorkerClientIdentifier.h. See for instance FetchOptions.h
(In reply to youenn fablet from comment #27) > > > Every time we receive a message, we will create a new ServiceWorker. > > > We should reuse them according their ids and if we create one, it should be > > > based on a full ServiceWorkerRegistration. > > > Can we fix that now? > > > > This is not an issue yet because we currently always reuse the controller. > > At some point, it'd be good to keep a list of ServiceWorkers somewhere I > > guess. > > Maybe we should add A FIXME and not implement this code path then. I do not agree. It is best to support the feature, even if not as efficient as it could be. Also note that a ServiceWorker is basically just a wrapper around an id and should be cheap to construct. > > > > > > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906 > > > > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier) > > > > > > The current style is to move this code closer to the definition, i.e. > > > ServiceWorkerClientIdentifier. > > > > Can you please clarify this? > > Add the encoder/decoder in ServiceWorkerClientIdentifier.h. > See for instance FetchOptions.h Oh OK, I can do this in a follow-up. I did not know this policy.
> > Maybe we should add A FIXME and not implement this code path then. > > I do not agree. It is best to support the feature, even if not as efficient > as it could be. Also note that a ServiceWorker is basically just a wrapper > around an id and should be cheap to construct. IINM, for every message in that situation, we will have a different JS source object which will be observable from JS. If so, we are not very compliant, right? Why not simply returning a null source? If we are testing this code path, it might be good to add test showing the behavior and whether this is expected.
(In reply to youenn fablet from comment #29) > > > Maybe we should add A FIXME and not implement this code path then. > > > > I do not agree. It is best to support the feature, even if not as efficient > > as it could be. Also note that a ServiceWorker is basically just a wrapper > > around an id and should be cheap to construct. > > IINM, for every message in that situation, we will have a different JS > source object which will be observable from JS. If so, we are not very > compliant, right? > Why not simply returning a null source? > > If we are testing this code path, it might be good to add test showing the > behavior and whether this is expected. If the source is null, then you cannot message the source back, which is much worse than getting a different JS wrapper every time.
(In reply to Chris Dumez from comment #30) > (In reply to youenn fablet from comment #29) > > > > Maybe we should add A FIXME and not implement this code path then. > > > > > > I do not agree. It is best to support the feature, even if not as efficient > > > as it could be. Also note that a ServiceWorker is basically just a wrapper > > > around an id and should be cheap to construct. > > > > IINM, for every message in that situation, we will have a different JS > > source object which will be observable from JS. If so, we are not very > > compliant, right? > > Why not simply returning a null source? > > > > If we are testing this code path, it might be good to add test showing the > > behavior and whether this is expected. > > If the source is null, then you cannot message the source back, which is > much worse than getting a different JS wrapper every time. Also, if you look at the spec, you'll see that it says to create a new ServiceWorker object. Therefore, it may just be compliant.
<rdar://problem/35568118>