Bug 233471 – Preload navigation request if the service worker is not immediately ready to handle the navigation request fetch event

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

[youenn fablet]

Preload navigation request if the service worker is not immediately ready to handle the navigation request fetch event

[Thomas Steiner]

Is this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852?

[youenn fablet]

Created attachment 445090 [details]
Patch

[youenn fablet]

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

[youenn fablet]

Created attachment 445130 [details]
Patch

[youenn fablet]

Created attachment 445200 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 445394 [details]
Patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/85857877>