Bug 233523 - Add support for NavigationPreloadManager
Summary: Add support for NavigationPreloadManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 233490
Blocks: 182852 233698 233751
  Show dependency treegraph
 
Reported: 2021-11-26 07:16 PST by youenn fablet
Modified: 2021-12-02 00:58 PST (History)
15 users (show)

See Also:


Attachments
Patch (111.32 KB, patch)
2021-11-26 07:29 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (127.06 KB, patch)
2021-12-01 05:14 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (127.62 KB, patch)
2021-12-01 06:57 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (130.76 KB, patch)
2021-12-01 23:34 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (130.97 KB, patch)
2021-12-01 23:59 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-11-26 07:16:08 PST
Add support for NavigationPreloadManager
Comment 1 youenn fablet 2021-11-26 07:29:22 PST
Created attachment 445202 [details]
Patch
Comment 2 Thomas Steiner 2021-11-26 09:25:06 PST
This now looks like a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852 for good :-)
Comment 3 youenn fablet 2021-12-01 05:14:23 PST
Created attachment 445562 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-12-01 05:43:45 PST
<rdar://problem/85918550>
Comment 5 youenn fablet 2021-12-01 06:57:57 PST
Created attachment 445572 [details]
Patch
Comment 6 Chris Dumez 2021-12-01 09:29:23 PST
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.
Comment 7 youenn fablet 2021-12-01 23:19:49 PST
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 { }.
Comment 8 youenn fablet 2021-12-01 23:34:13 PST
Created attachment 445679 [details]
Patch for landing
Comment 9 youenn fablet 2021-12-01 23:59:34 PST
Created attachment 445681 [details]
Patch for landing
Comment 10 EWS 2021-12-02 00:57:59 PST
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].