Storage Process needs to know potential ServiceWorker clients. Any document which may interact with a service worker should be registered.
Created attachment 327654 [details] Patch
Created attachment 327663 [details] Patch
Created attachment 327664 [details] Patch
Created attachment 327667 [details] Patch
Comment on attachment 327667 [details] Patch Attachment 327667 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5378744 Number of test failures exceeded the failure limit.
Created attachment 327675 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 327667 [details] Patch Attachment 327667 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5378697 Number of test failures exceeded the failure limit.
Created attachment 327677 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 327667 [details] Patch Attachment 327667 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5378877 Number of test failures exceeded the failure limit.
Created attachment 327680 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 327667 [details] Patch Attachment 327667 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5379142 Number of test failures exceeded the failure limit.
Created attachment 327682 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 327699 [details] Patch
Created attachment 327711 [details] Patch
Created attachment 327775 [details] Patch
Comment on attachment 327775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327775&action=review > Source/WebCore/ChangeLog:17 > + Storing all clients in SWServer as a HasMap keyed by ClientOrigin. typo: HasMap > Source/WebCore/dom/Document.cpp:7589 > +void Document::setServiceWorkerConnection(SWClientConnection* serviceWorkerConnection) This seems to be only called when a service worker starts controlling a document. This seems wrong no? In the current state, it seems identical to the "client using registration" concept which we already have. > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:40 > + bool operator==(const ServiceWorkerClientIdentifier& identifier) const { return serverConnectionIdentifier == identifier.serverConnectionIdentifier && contextIdentifier == identifier.contextIdentifier; } Darin told me we normally prefer when the operator is outside the class, not a member. > Source/WebCore/workers/service/server/SWServer.cpp:502 > + clients.removeFirstMatching([&] (const auto& item) { Why aren't we using a HashSet? > Source/WebCore/workers/service/server/SWServer.h:177 > + HashMap<ClientOrigin, Vector<ClientInformation>> m_clients; Why are clients stored like this and not on SWServiceWorker? Those are *service worker* clients, right? If so, shouldn't they be tried to a SWServiceWorker object instead of globally on the server? > Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:99 > + HashMap<WebCore::DocumentIdentifier, WebCore::ClientOrigin> m_clientOrigins; Why do we need another copy of this map? This is basically the same map as on SWServer, no?
Basically, I expected something like: HashSet<ServiceWorkerClientIdentifier> m_clients; on SWServerWorker. Any reason we cannot / should not do this?
Comment on attachment 327775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327775&action=review >> Source/WebCore/dom/Document.cpp:7589 >> +void Document::setServiceWorkerConnection(SWClientConnection* serviceWorkerConnection) > > This seems to be only called when a service worker starts controlling a document. This seems wrong no? In the current state, it seems identical to the "client using registration" concept which we already have. If service worker runtime flag is on, it will be called for every document in DocumentLoader::commitData. This patch does the registration as a client just before selecting a service worker if there is one. >> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:40 >> + bool operator==(const ServiceWorkerClientIdentifier& identifier) const { return serverConnectionIdentifier == identifier.serverConnectionIdentifier && contextIdentifier == identifier.contextIdentifier; } > > Darin told me we normally prefer when the operator is outside the class, not a member. Right. >> Source/WebCore/workers/service/server/SWServer.cpp:502 >> + clients.removeFirstMatching([&] (const auto& item) { > > Why aren't we using a HashSet? Was not too sure about it. I picked a Vector since algos like Clients.matchAll are iterating through this list and are generating arrays of clients. That said, matchAll is requiring some sorting at the end of the algorithm so maybe a HashSet would be better. >> Source/WebCore/workers/service/server/SWServer.h:177 >> + HashMap<ClientOrigin, Vector<ClientInformation>> m_clients; > > Why are clients stored like this and not on SWServiceWorker? Those are *service worker* clients, right? If so, shouldn't they be tried to a SWServiceWorker object instead of globally on the server? Not sure to follow here. They are indeed service worker clients and any service worker from that ClientOrigin can talk to any of this client. >> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:99 >> + HashMap<WebCore::DocumentIdentifier, WebCore::ClientOrigin> m_clientOrigins; > > Why do we need another copy of this map? This is basically the same map as on SWServer, no? This is a map specific to the clients of a WebProcess, SWServer map is global to all web processes. m_clientOrigins for instance allows removing all clients of a given WebProcess if it crashes for instance.
Comment on attachment 327775 [details] Patch Little details pointed out by Chris aside, I think this is the necessary approach
(In reply to Brady Eidson from comment #19) > Comment on attachment 327775 [details] > Patch > > Little details pointed out by Chris aside, I think this is the necessary > approach Yes, after discussing this offline with Youenn. My initial understanding was wrong and I am ok with this approach as well now.
Created attachment 327807 [details] Patch for landing
I kept a vector instead of a HashSet for now. I will revisit this when fully implementing Clients API.
Comment on attachment 327807 [details] Patch for landing Clearing flags on attachment: 327807 Committed r225251: <https://trac.webkit.org/changeset/225251>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35738152>
(In reply to youenn fablet from comment #22) > I kept a vector instead of a HashSet for now. > I will revisit this when fully implementing Clients API. Looking at it further, matched clients should be sorted by their creation order. This creation order is now supported by Vector. If we need a more explicit creation order, we should use a HashSet.