Bug 233523 – Add support for NavigationPreloadManager

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Add support for NavigationPreloadManager

[youenn fablet]

Created attachment 445202 [details]
Patch

[Thomas Steiner]

This now looks like a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852 for good :-)

[youenn fablet]

Created attachment 445562 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85918550>

[youenn fablet]

Created attachment 445572 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

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 { }.

[youenn fablet]

Created attachment 445679 [details]
Patch for landing

[youenn fablet]

Created attachment 445681 [details]
Patch for landing

[EWS]

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].