Bug 179554 - Massive "Server-process-to-context-process" connection overhaul
Summary: Massive "Server-process-to-context-process" connection overhaul
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-10 15:14 PST by Brady Eidson
Modified: 2017-11-15 09:36 PST (History)
8 users (show)

See Also:


Attachments
WIP patch (99.34 KB, patch)
2017-11-13 12:54 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (99.32 KB, patch)
2017-11-13 13:20 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
WIP (102.28 KB, patch)
2017-11-13 14:09 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
WIP (102.29 KB, patch)
2017-11-13 14:23 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
PFR (113.39 KB, patch)
2017-11-13 14:49 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (113.43 KB, patch)
2017-11-13 15:00 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (138.20 KB, patch)
2017-11-13 16:28 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (138.31 KB, patch)
2017-11-13 16:36 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (136.33 KB, patch)
2017-11-13 18:47 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-11-10 15:14:24 PST
Add standalone WebCore abstraction for "connection to worker context process"

Currently only the StorageProcess in WebKit knows about this. WebCore needs to know.
Comment 1 Brady Eidson 2017-11-12 20:22:23 PST
Rename:

Massive "Server-process-to-context-process" overhaul

There was so much unused and unnecessary code here, and the new model will make so much sense going forward.
Comment 2 Brady Eidson 2017-11-13 12:54:23 PST Comment hidden (obsolete)
Comment 3 Brady Eidson 2017-11-13 13:20:45 PST Comment hidden (obsolete)
Comment 4 Brady Eidson 2017-11-13 14:09:52 PST Comment hidden (obsolete)
Comment 5 Brady Eidson 2017-11-13 14:23:31 PST Comment hidden (obsolete)
Comment 6 Brady Eidson 2017-11-13 14:49:30 PST Comment hidden (obsolete)
Comment 7 Brady Eidson 2017-11-13 15:00:28 PST
Created attachment 326809 [details]
Patch
Comment 8 Brady Eidson 2017-11-13 15:43:53 PST
I have the encodeEnum thing cleaned up locally, please keep reviewing.
Comment 9 Chris Dumez 2017-11-13 16:17:45 PST
Comment on attachment 326809 [details]
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.
Comment 10 Brady Eidson 2017-11-13 16:28:07 PST
Created attachment 326820 [details]
Patch
Comment 11 WebKit Commit Bot 2017-11-13 16:30:42 PST
Comment on attachment 326820 [details]
Patch

Rejecting attachment 326820 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 326820, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 succeeded at 8213 (offset 2 lines).
Hunk #7 succeeded at 9138 (offset 2 lines).
Hunk #8 succeeded at 10805 (offset 2 lines).
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/5218446
Comment 12 Brady Eidson 2017-11-13 16:36:56 PST
Created attachment 326821 [details]
Patch
Comment 13 Brady Eidson 2017-11-13 18:38:58 PST
UGH rebase nightmares.
Comment 14 Brady Eidson 2017-11-13 18:47:15 PST
Created attachment 326837 [details]
Patch
Comment 15 WebKit Commit Bot 2017-11-13 19:58:06 PST
Comment on attachment 326837 [details]
Patch

Clearing flags on attachment: 326837

Committed r224801: <https://trac.webkit.org/changeset/224801>
Comment 16 WebKit Commit Bot 2017-11-13 19:58:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2017-11-15 09:36:45 PST
<rdar://problem/35562068>