This is a first step towards simplifying service worker loads and optimise the ping pong we are doing for service worker matching.
Created attachment 379721 [details] Patch
Created attachment 380647 [details] Patch
Created attachment 380734 [details] Patch
Created attachment 380741 [details] Patch
Created attachment 380747 [details] Patch
Created attachment 380748 [details] Patch
Created attachment 380749 [details] Patch
Created attachment 380751 [details] Patch
Comment on attachment 380751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380751&action=review > Source/WebCore/workers/service/server/SWServer.cpp:977 > +bool SWServer::canHandleScheme(const String& scheme) const I would make this take a StringView, then have a fast path if m_registeredSchemes.isEmpty() that doesn't make a String or hash it at all. That will reduce a string construction and hash in the common non-testing case. > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:265 > + Optional<HashSet<HTTPHeaderName, WTF::IntHash<HTTPHeaderName>, WTF::StrongEnumHashTraits<HTTPHeaderName>>> httpHeadersToKeep; I think you should typedef this HashSet to avoid repeating so many template parameters. Also, would it be worth just sending a Vector, or is hashing it worth it? > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:111 > -void ServiceWorkerFetchTask::didReceiveResponse(const ResourceResponse& response, bool needsContinueDidReceiveResponseMessage) > +void ServiceWorkerFetchTask::didReceiveResponse(ResourceResponse&& response, bool needsContinueDidReceiveResponseMessage) I don't see why you're moving a response into this function, which only sends it over IPC. It seems unnecessary. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:130 > - if (m_connection) > - m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveFormData { formData }, m_identifier.fetchIdentifier); > + // FIXME: Allow WebResourceLoader to receive form data. This seems like a regression as it stands now.
(In reply to Alex Christensen from comment #9) > Comment on attachment 380751 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380751&action=review > > > Source/WebCore/workers/service/server/SWServer.cpp:977 > > +bool SWServer::canHandleScheme(const String& scheme) const > > I would make this take a StringView, then have a fast path if > m_registeredSchemes.isEmpty() that doesn't make a String or hash it at all. > That will reduce a string construction and hash in the common non-testing > case. I hesitated and went with const String& since this was the way it was done before. Will change to StringView. > > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:265 > > + Optional<HashSet<HTTPHeaderName, WTF::IntHash<HTTPHeaderName>, WTF::StrongEnumHashTraits<HTTPHeaderName>>> httpHeadersToKeep; > > I think you should typedef this HashSet to avoid repeating so many template > parameters. Should use HTTPHeaderNameSet. > Also, would it be worth just sending a Vector, or is hashing it worth it? Right, we could probably use a Vector, this should be done independently though since this should be changed in DocumntThreadableLoader et al. > > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:111 > > -void ServiceWorkerFetchTask::didReceiveResponse(const ResourceResponse& response, bool needsContinueDidReceiveResponseMessage) > > +void ServiceWorkerFetchTask::didReceiveResponse(ResourceResponse&& response, bool needsContinueDidReceiveResponseMessage) > > I don't see why you're moving a response into this function, which only > sends it over IPC. It seems unnecessary. We cannot use a const since we are modifying the response. a ResourceResponse& is sufficient though. > > > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:130 > > - if (m_connection) > > - m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveFormData { formData }, m_identifier.fetchIdentifier); > > + // FIXME: Allow WebResourceLoader to receive form data. > > This seems like a regression as it stands now. It is not since current code base has a FIXME for that in ServiceWorkerClientFetch::didReceiveFormData.
Created attachment 380860 [details] Patch
Created attachment 380863 [details] Patch
Comment on attachment 380863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380863&action=review > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:178 > + Vector<ServiceWorkerFetchTask*> fetches; Since we're no longer mutating m_ongoingFetches while iterating, could we remove the use of the Vector? If not, could we make this a safer pointer type? If not, could we at least use reserveInitialCapacity/uncheckedAppend?
(In reply to Alex Christensen from comment #13) > Comment on attachment 380863 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380863&action=review > > > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:178 > > + Vector<ServiceWorkerFetchTask*> fetches; > > Since we're no longer mutating m_ongoingFetches while iterating, could we > remove the use of the Vector? If not, could we make this a safer pointer > type? If not, could we at least use reserveInitialCapacity/uncheckedAppend? Creating a Vector is safer as calling fail/didNotHandle will mutate m_ongoingFetches. We could use a Vector<WeakPtr<>> but we would need to increment the weak ptr ref or move it from the map to the vector. Let me know if you prefer that. We could use reserveInitialCapacity but we might over-allocate since only the fetches of one worker only will be put in the vector. We usually do not do that. We could also simply copyToVector the map and iterate over it. Probably not a big deal for perf since the time out should happen rarely.
Comment on attachment 380863 [details] Patch Cq+ing, I can update the patch for the fetch vector in a follow-up.
Comment on attachment 380863 [details] Patch Clearing flags on attachment: 380863 Committed r251124: <https://trac.webkit.org/changeset/251124>
All reviewed patches have been landed. Closing bug.
Seems to break again GTK bot, probably ENABLE(SERVICE_WORKER) issues. Will look at it.
Reopening to attach new patch.
Created attachment 380968 [details] GTK build fix
<rdar://problem/56283603>
Comment on attachment 380968 [details] GTK build fix Clearing flags on attachment: 380968 Committed r251126: <https://trac.webkit.org/changeset/251126>
(In reply to WebKit Commit Bot from comment #16) > Comment on attachment 380863 [details] > Patch > > Clearing flags on attachment: 380863 > > Committed r251124: <https://trac.webkit.org/changeset/251124> Seems this has caused lot of crashes in service-workers tests on GTK. The bot is aborting early. I will try to get a backtrack.
(In reply to Carlos Alberto Lopez Perez from comment #24) > (In reply to WebKit Commit Bot from comment #16) > > Comment on attachment 380863 [details] > > Patch > > > > Clearing flags on attachment: 380863 > > > > Committed r251124: <https://trac.webkit.org/changeset/251124> > > Seems this has caused lot of crashes in service-workers tests on GTK. > > The bot is aborting early. > > I will try to get a backtrack. Reportad that on bug 203036
(In reply to Carlos Alberto Lopez Perez from comment #25) > (In reply to Carlos Alberto Lopez Perez from comment #24) > > (In reply to WebKit Commit Bot from comment #16) > > > Comment on attachment 380863 [details] > > > Patch > > > > > > Clearing flags on attachment: 380863 > > > > > > Committed r251124: <https://trac.webkit.org/changeset/251124> > > > > Seems this has caused lot of crashes in service-workers tests on GTK. > > > > The bot is aborting early. > > > > I will try to get a backtrack. > > Reportad that on bug 203036 I'll get a look tomorrow.