[webkit-reviews] review granted: [Bug 233523] Add support for NavigationPreloadManager : [Attachment 445572] 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 233523: Add support for NavigationPreloadManager
https://bugs.webkit.org/show_bug.cgi?id=233523

Attachment 445572: Patch

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




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