[webkit-reviews] review granted: [Bug 53192] Add experimental support for HTTP pipelining in CFNetwork : [Attachment 80237] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 53192: Add experimental support for HTTP pipelining in CFNetwork
https://bugs.webkit.org/show_bug.cgi?id=53192

Attachment 80237: Patch
https://bugs.webkit.org/attachment.cgi?id=80237&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=80237&action=review

Looks good, r=me with build fixes + stylebot fixes + a comment below.

> Source/WebCore/loader/ResourceLoadScheduler.cpp:124
> +    if (isHttpPipeliningEnabled()) {
> +	   // Serve all requests at once to keep the pipeline full at the
network layer.
> +	   servePendingRequests(host, ResourceLoadPriorityLowest);
> +	   return;
> +    }

This change looks like an optimization rather than something that is needed for
the patch to work. Did you benchmark it? It might actually hurt performance in
some cases (by making us load images before more important resources). It would
probably be better do it separately if it actually helps.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:116
>      ResourceLoadPriority priority = resource->loadPriority();
> +    resourceRequest.setPriority(priority);

It would be nicer if the ResourceLoadScheduler would set this. That would allow
us to choose in one place how much networking layer level prioritization we
want. But I see the request is const so this is fine for this patch.