[webkit-reviews] review granted: [Bug 229465] [WK2] Reuse the same network load when process-swapping on resource response due to COOP : [Attachment 436556] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 229465: [WK2] Reuse the same network load when process-swapping on resource
response due to COOP
https://bugs.webkit.org/show_bug.cgi?id=229465

Attachment 436556: Patch

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




--- Comment #18 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 436556
  --> https://bugs.webkit.org/attachment.cgi?id=436556
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.h:620
> +	   Ref<NetworkResourceLoader> takeLoader();

There's no benefit in making this return a Ref.  It should return a RefPtr. 
Then we don't need an unchecked m_loader.releaseNonNull.  If you want the
assertion, ASSERT(m_loader);

> Source/WebKit/NetworkProcess/NetworkProcess.h:627
> +    HashMap<NetworkResourceLoadIdentifier,
std::unique_ptr<CachedNetworkResourceLoader>>
m_loadersAwaitingWebProcessTransfer;

I think this should be owned by the NetworkSession to compartmentalize things a
little bit more.  There isn't a good reason this needs to be process-global.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:217
> +    NetworkResourceLoadParameters m_parameters;

I think I'd prefer to mark m_parameters.identifier as mutable rather than this,
which indicates that any member can be mutated.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:95
> +    void
setExistingNetworkResourceLoadIdentifierToResume(std::optional<NetworkResourceL
oadIdentifier> existingNetworkResourceLoadIdentifierToResume) {
m_existingNetworkResourceLoadIdentifierToResume =
existingNetworkResourceLoadIdentifierToResume; }

This is kind of a verbose name already, but shouldn't this indicate that it is
only for the main resource of the main frame?

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:852
> +    uint64_t mainResourceLoadIdentifier = policyDocumentLoader &&
policyDocumentLoader->mainResourceLoader() ?
policyDocumentLoader->mainResourceLoader()->identifier() : 0;

ResourceLoader::identifier returns an unsigned long.  Would auto work here?
Add FIXME: use a strongly typed identifier.  We don't want to merge that
refactoring, but we need to do it.