[webkit-reviews] review granted: [Bug 184763] Make WebLoaderStrategy send to NetworkResourceLoader necessary parameters to handle full loads in NetworkProcess : [Attachment 338324] 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 184763: Make WebLoaderStrategy send to NetworkResourceLoader necessary
parameters to handle full loads in NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=184763

Attachment 338324: Patch

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




--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 338324
  --> https://bugs.webkit.org/attachment.cgi?id=338324
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338324&action=review

r=me

> Source/WebCore/ChangeLog:10
> +	  Add PreflightPolicy and CSP response headers as
ResourceLoaderOptions.

Indentation problem.

> Source/WebCore/ChangeLog:13
> +	  Allow getting the original headers from a SubresourceLoader.

ditto.

> Source/WebCore/loader/ResourceLoaderOptions.h:116
> +    ConsiderPreflight,

I think we could even consider dropping the "Preflight" suffix given the enum
name.

> Source/WebCore/loader/SubresourceLoader.cpp:787
> +std::optional<HTTPHeaderMap> SubresourceLoader::originalHeaders() const

If we returned a HTTPHeaderMap* instead of an std::optional<>, we could avoid
copying the headers.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:287
> +    if (resourceLoader.isSubresourceLoader()) {

It would be nice to add the template specializations so that
is<SubresourceLoader>() / downcast<SubresourceLoader>() works. Would be safer
too.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:289
> +	       loadParameters.originalRequestHeaders =
WTFMove(headers.value());

I get that we have to copy the headers here anyway, I'd still prefer if the API
on SubresourceLoader did not requires the clients to in case they do not
need/want copying.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:295
> +	   // FIXME: Instead of passing userContentControllerIdentifier, we
should just pass webPageId to NetworkProcess.

I thought we already passed the PageID / FrameID to the network process so I do
not understand this comment.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:296
> +	   WebFrameLoaderClient* webFrameLoaderClient =
toWebFrameLoaderClient(resourceLoader.frame()->loader().client());

auto*

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:297
> +	   WebFrame* webFrame = webFrameLoaderClient ?
webFrameLoaderClient->webFrame() : nullptr;

auto*

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:298
> +	   WebPage* webPage = webFrame ? webFrame->page() : nullptr;

auto*