Remove mayHaveServiceWorkerRegisteredForOrigin
Created attachment 381112 [details] Patch
Note: I think this was originally introduced to prevent a storage process from being started on startup. That reason for existence is gone since the storage process was merged into the network process.
Comment on attachment 381112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381112&action=review > Source/WebCore/ChangeLog:9 > + It used to be significant when more service worker logic was in the UIProcess, but now that it's in the network process, It was in the StorageProcess. This was an optimization by Youenn (I related the bugs). His claim was: "This may help speeding up the load of the first page in the web process."
I'll update the ChangeLog before landing.
I have no idea why this affects only iOS
Comment on attachment 381112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381112&action=review > Source/WebCore/ChangeLog:3 > + Remove mayHaveServiceWorkerRegisteredForOrigin I do not think we are ready yet to remove it. Without it, we would go to network process to check for registration and wait for the registration result to start the load. This is not great when we know there is no service worker. The fact that StorageProcess is no longer there reduces the optimization benefit but there is still a benefit. There are in fact two optimisations: 1. First optimization is to check for swOriginTable to see whether a service worker is registered for that origin. 2. Second optimization is, when swOriginTable is not yet initialised (which happens at the beginning of the web process), to check whether any service worker is registered. I am working on moving the registration matching from DocumentLoader to NetworkResourceLoader. At that point, we will be able to remove this optimization since everything will be done in NetworkProcess. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1115 > + NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"sw://host/regularPageWithConnection.html"]]; This test is good to have. Currently, we are checking registration before custom schemes. When moving registration matching to network process, I will need to go back to web process in case registration is not matching and there is a custom scheme. It would be simpler if custom schemes were loaded from NetworkProcess or if we were removing support for service workers interception of custom schemes.
(In reply to youenn fablet from comment #6) > I am working on moving the registration matching from DocumentLoader to > NetworkResourceLoader. > At that point, we will be able to remove this optimization since everything > will be done in NetworkProcess. I look forward to it. I guess I'm blocked on it. > It would be simpler if custom schemes were loaded from NetworkProcess or if > we were removing support for service workers interception of custom schemes. I don't think there's anyone using service workers to intercept custom schemes.
(In reply to Alex Christensen from comment #7) > (In reply to youenn fablet from comment #6) > > I am working on moving the registration matching from DocumentLoader to > > NetworkResourceLoader. > > At that point, we will be able to remove this optimization since everything > > will be done in NetworkProcess. > I look forward to it. I guess I'm blocked on it. > > > It would be simpler if custom schemes were loaded from NetworkProcess or if > > we were removing support for service workers interception of custom schemes. > I don't think there's anyone using service workers to intercept custom > schemes. Agree it is a bit of an edge case. API tests are doing so right now to work around the lack of a HTTP server.
(In reply to youenn fablet from comment #8) > (In reply to Alex Christensen from comment #7) > > (In reply to youenn fablet from comment #6) > > > I am working on moving the registration matching from DocumentLoader to > > > NetworkResourceLoader. > > > At that point, we will be able to remove this optimization since everything > > > will be done in NetworkProcess. > > I look forward to it. I guess I'm blocked on it. > > > > > It would be simpler if custom schemes were loaded from NetworkProcess or if > > > we were removing support for service workers interception of custom schemes. > > I don't think there's anyone using service workers to intercept custom > > schemes. > > Agree it is a bit of an edge case. > API tests are doing so right now to work around the lack of a HTTP server. Luckily we now have TCPServer.
(In reply to Alex Christensen from comment #9) > Luckily we now have TCPServer. I'll prepare a patch.
https://bugs.webkit.org/show_bug.cgi?id=203141
> I am working on moving the r egistration matching from DocumentLoader to > NetworkResourceLoader. https://bugs.webkit.org/show_bug.cgi?id=203144 > At that point, we will be able to remove this optimization since everything > will be done in NetworkProcess. Removing the sworiginstore optimization might have a slight cost for some cases (appcache loads and memory cache loads). Should investigate to see whether this is fine. We will be able to remove WebProcessPool::mayHaveRegisteredServiceWorkers.
Created attachment 381496 [details] Patch
Comment on attachment 381496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381496&action=review > Source/WebCore/loader/DocumentLoader.cpp:-509 > - if (!provider.mayHaveServiceWorkerRegisteredForOrigin(origin)) { Let's replace this one for now by: if (!ServiceWorkerProvider::singleton().serviceWorkerConnection().mayHaveServiceWorkerRegisteredForOrigin(origin)) We should also remove existingServiceWorkerConnection() since we no longer care about the cost of creating a connection. > Source/WebCore/workers/service/SWClientConnection.h:-80 > - virtual bool mayHaveServiceWorkerRegisteredForOrigin(const SecurityOriginData&) const = 0; Let's keep this one. > Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:-116 > -bool WebSWClientConnection::mayHaveServiceWorkerRegisteredForOrigin(const SecurityOriginData& origin) const Let's not remove this one for now. It seems best to remove this mechanism in its own revision ,for regression analysis. Also, when removing this one, we should remove more code when doing so in SWServer et al.
Created attachment 381525 [details] Patch
Created attachment 381544 [details] Patch
Created attachment 381550 [details] retry ews
(In reply to Alex Christensen from comment #17) > Created attachment 381550 [details] > retry ews EWS now has the ability to retry patches, just click the 'Retry failed builds' button. See https://lists.webkit.org/pipermail/webkit-dev/2019-October/030888.html
But does it have the ability to download builds from itself?
(In reply to Alex Christensen from comment #19) > But does it have the ability to download builds from itself? Currently some of the bots are facing network issues while trying to reach Amazon S3 https://lists.webkit.org/pipermail/webkit-dev/2019-October/030887.html We are actively looking in the issue.
http://trac.webkit.org/r251439
<rdar://problem/56504295>
Committed r251470: <https://trac.webkit.org/changeset/251470>