Preload navigation request if the service worker is not immediately ready to handle the navigation request fetch event
Is this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852?
Created attachment 445090 [details] Patch
(In reply to Thomas Steiner from comment #1) > Is this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852? Not really, this is restricted to the case where the service worker is not running. When service worker is running, going to the service worker is usually fast: added delay to the network is a few milliseconds. When service worker is not running, the delay is larger, depending on the size of the service worker script.
Created attachment 445130 [details] Patch
Created attachment 445200 [details] Patch
Comment on attachment 445200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445200&action=review r=me with nits > Source/WebKit/ChangeLog:10 > + To optimize this code path, we preload the navigation request in case service worker ins not running or not yet activated. typo: ins > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:82 > + if (m_preloader) { This logic comes up so many times, we may want to consider moving it to a cancelPreloadIfNecessary() function. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:345 > + m_preloader->waitForResponse([weakThis = WeakPtr { this }, this]() { WeakPtr { *this } is slightly better as it avoids a null check. `()` is not needed. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:370 > + m_preloader->waitForBody([weakThis = WeakPtr { this }, this](auto&& chunk, int length) { WeakPtr { *this } > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:121 > + bool m_isLoadingFromPreloader { false }; We may want to group the boolean data members together for better padding. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:77 > +ServiceWorkerNavigationPreloader::~ServiceWorkerNavigationPreloader() = default; > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:92 > + didReceiveResponse(ResourceResponse { entry.response() }, [body = RefPtr { entry.buffer() }, weakThis = WeakPtr { this }](auto) mutable { WeakPtr { *this } > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:132 > + auto cacheEntry = WTFMove(m_cacheEntry); Not sure using a local variable improves things here. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:142 > + auto callback = std::exchange(m_responseCallback, { }); This could go inside the condition: ``` if (auto callback = std::exchange(m_responseCallback, { })) callback(); ``` > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:174 > + auto callback = std::exchange(m_responseCallback, { }); This could go inside the if condition. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h:52 > + using ResponseCallback = Function<void()>; Is this a Function instead of a CompletionHandler because it might not get called? It feels like it could be a CompletionHandler otherwise.
Thanks for the review, will update patch accordingly. > > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h:52 > > + using ResponseCallback = Function<void()>; > > Is this a Function instead of a CompletionHandler because it might not get > called? > It feels like it could be a CompletionHandler otherwise. Yes, if preloaded gets cancelled, it does not seem great to call this callback.
Created attachment 445394 [details] Patch for landing
Committed r286288 (244647@main): <https://commits.webkit.org/244647@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445394 [details].
<rdar://problem/85857877>