[webkit-reviews] review granted: [Bug 233471] Preload navigation request if the service worker is not immediately ready to handle the navigation request fetch event : [Attachment 445200] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 233471: Preload navigation request if the service worker is not immediately
ready to handle the navigation request fetch event
https://bugs.webkit.org/show_bug.cgi?id=233471

Attachment 445200: Patch

https://bugs.webkit.org/attachment.cgi?id=445200&action=review




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 445200
  --> https://bugs.webkit.org/attachment.cgi?id=445200
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:5
2
> +    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.