[webkit-reviews] review granted: [Bug 178794] Add initial support for serviceWorkerClient.postMessage() : [Attachment 325099] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 26 22:36:20 PDT 2017


youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 178794: Add initial support for serviceWorkerClient.postMessage()
https://bugs.webkit.org/show_bug.cgi?id=178794

Attachment 325099: Patch

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




--- Comment #22 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 325099
  --> https://bugs.webkit.org/attachment.cgi?id=325099
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.


More information about the webkit-reviews mailing list