Add support for NavigationPreloadManager
Created attachment 445202 [details] Patch
This now looks like a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852 for good :-)
Created attachment 445562 [details] Patch
<rdar://problem/85918550>
Created attachment 445572 [details] Patch
Comment on attachment 445572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445572&action=review r=me with comments/questions. > Source/WebCore/Modules/cache/DOMCache.cpp:305 > + }, "fetch"_s); Why not cachedResourceRequestInitiators().fetch ? > Source/WebCore/Modules/fetch/WindowOrWorkerGlobalScopeFetch.cpp:65 > + }, "fetch"_s); ditto. > Source/WebCore/Modules/fetch/WindowOrWorkerGlobalScopeFetch.cpp:80 > + }, "fetch"_s); ditto. > Source/WebCore/workers/service/FetchEvent.cpp:173 > + }, "navigation"_s); Do we want to add navigation to CachedResourceRequestInitiators for consistency? > Source/WebCore/workers/service/NavigationPreloadState.h:58 > + return NavigationPreloadState { *enabled, WTFMove(*headerValue) }; Could be: return { { *enabled, WTFMove(*headerValue) } }; > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:678 > + promise.resolve(result.releaseReturnValue()); Why doesn't settle() work here? > Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:171 > + if (!isServiceWorkerNavigationPreloadEnabled) A little surprised by the ! here. Could you please double check it is right? > Source/WebCore/workers/service/server/SWServerRegistration.h:117 > + NavigationPreloadState navigationPreloadState() { return m_preloadState; } Seems like this function should be const and should return a `const NavigationPreloadState&` > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:147 > + m_parameters.request.addHTTPHeaderField("Service-Worker-Navigation-Preload"_s, m_state.headerValue); We may want to add this new header to HTTPHeaderNames.in instead of using a string literal. > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h:87 > + bool m_shouldCaptureExtraNetworkLoadMetrics { false }; We may want to group the boolean data members together for better packing. > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:530 > + callback(ExceptionData { InvalidStateError, { "No registration"_s } }); Why the { } around the "No registration"_s ? If the idea is to construct a String and the compiler really doesn't want to build without it, then maybe "No registration"_str would work. Same comment below in this file.
Thanks for the review. > Why not cachedResourceRequestInitiators().fetch ? OK > Do we want to add navigation to CachedResourceRequestInitiators for > consistency? Will do. > > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:678 > > + promise.resolve(result.releaseReturnValue()); > > Why doesn't settle() work here? settle expects a ExceptionOr<const NavigationPreloadState&> and we provide a ExceptionOr<NavigationPreloadState>. I guess we could improve settle to support this by adding a version with StorageType. > > Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:171 > > + if (!isServiceWorkerNavigationPreloadEnabled) > > A little surprised by the ! here. Could you please double check it is right? Will add a comment. We cannot reuse a service worker navigation preload when fetching the request directly in case there is a Vary: Service-Worker-Navigation-Preload. > > Source/WebCore/workers/service/server/SWServerRegistration.h:117 > > + NavigationPreloadState navigationPreloadState() { return m_preloadState; } > > Seems like this function should be const and should return a `const > NavigationPreloadState&` Yes, I was fixing this in a follow-up patch to add persistency but will add it there. > > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:147 > > + m_parameters.request.addHTTPHeaderField("Service-Worker-Navigation-Preload"_s, m_state.headerValue); > > We may want to add this new header to HTTPHeaderNames.in instead of using a > string literal. OK. I was not exactly sure whether that was useful enough, given we will never query this header for instance. > > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h:87 > > + bool m_shouldCaptureExtraNetworkLoadMetrics { false }; > > We may want to group the boolean data members together for better packing. OK > > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:530 > > + callback(ExceptionData { InvalidStateError, { "No registration"_s } }); > > Why the { } around the "No registration"_s ? If the idea is to construct a > String and the compiler really doesn't want to build without it, then maybe > "No registration"_str would work. > > Same comment below in this file. Not sure why, yes, I'll remove { }.
Created attachment 445679 [details] Patch for landing
Created attachment 445681 [details] Patch for landing
Committed r286419 (244765@main): <https://commits.webkit.org/244765@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445681 [details].