[webkit-reviews] review denied: [Bug 229686] Preconnected socket is sometimes not used for initial request : [Attachment 437527] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Ben Nham
<nham at apple.com>'s request for review:
Bug 229686: Preconnected socket is sometimes not used for initial request
https://bugs.webkit.org/show_bug.cgi?id=229686

Attachment 437527: Patch

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




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

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

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.cpp:196
> +void NetworkLoadScheduler::startedPreconnectForMainResource(const URL& url)

This seems like a lot of logic for scheduling http1 preloads sequentially.  I
would think this logic should go in CFNetwork checking if it has a preconnect
task in flight instead of WebKit implementing its own scheduler.

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.h:74
> +	   ListHashSet<NetworkLoad *> pendingLoads;

We shouldn't be storing raw pointers like this.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1397
> +    if (reason == PreconnectReason::MainResourceLoad) {
> +	  
parameters.request.setPriority(WebCore::ResourceLoadPriority::VeryHigh);

Is there a reason we don't want to do this for preconnects from the API?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1405
> +    task->setTimeout(1_s);

This seems too short to make preconnects useful in the real internet.

> Source/WebKit/NetworkProcess/PreconnectReason.h:33
> +enum class PreconnectReason : uint8_t {

: bool
Then you don't need EnumTraits below because it already knows 0 and 1 are the
only valid values.

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:-50
> -    m_timeoutTimer.startOneShot(60000_s);

This was definitely unnecessarily long.

> Source/WebKit/NetworkProcess/PreconnectTask.h:32
> +#include <optional>

This looks unused.