[webkit-reviews] review granted: [Bug 179554] Massive "Server-process-to-context-process" connection overhaul : [Attachment 326809] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 179554: Massive "Server-process-to-context-process" connection overhaul
https://bugs.webkit.org/show_bug.cgi?id=179554

Attachment 326809: Patch

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




--- Comment #9 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 326809
  --> https://bugs.webkit.org/attachment.cgi?id=326809
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326809&action=review

r=me with comments.

> Source/WebCore/workers/service/ServiceWorkerContextData.h:54
> +    encoder.encodeEnum(workerType);

Doesn't << work for enums?

> Source/WebCore/workers/service/server/SWServer.cpp:313
> +void SWServer::fireInstallEvent(ServiceWorkerIdentifier
serviceWorkerIdentifier)

It can be done in a follow up but this is a bit unfortunate because the call
site of this method has a SWServerWorker object. The call site should pass in
this object instead of merely the identifier.

> Source/WebCore/workers/service/server/SWServer.cpp:330
> +void SWServer::fireActivateEvent(ServiceWorkerIdentifier
serviceWorkerIdentifier)

ditto.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:138
> +   
m_server.fireInstallEvent(registration->installingWorker()->identifier());

Here we should pass in *registration->installingWorker().

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:226
> +    server.fireActivateEvent(registration.activeWorker()->identifier());

Here we should pass in *registration.activeWorker().

> Source/WebCore/workers/service/server/SWServerToContextConnection.cpp:57
> +    auto taken = allConnections().take(m_identifier);

Just remove(), not take. renove() returns a boolean that you can check in the
assertion. Take() is unnecessarily inefficient here.

>
Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.h:51
> +    WebSWServerToContextConnection(Ref<IPC::Connection>&&);

explicit?

>
Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.h:57
> +    // Messages to the SW host WebProcess

Missing period at the end.