[webkit-reviews] review denied: [Bug 237579] [Curl] Implement WebSocketTask : [Attachment 454391] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 237579: [Curl] Implement WebSocketTask
https://bugs.webkit.org/show_bug.cgi?id=237579

Attachment 454391: Patch

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




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

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

Looks mostly good.  Thanks for upstreaming.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:46
> +    if (request.url().protocolIs("wss") &&
WebCore::DeprecatedGlobalSettings::allowsAnySSLCertificate())
> +	  
WebCore::CurlContext::singleton().sslHandle().setIgnoreSSLErrors(true);

It would be great if we could do this per task or at least per domain.	That's
for a different patch, though.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:55
> +WebSocketTask::~WebSocketTask()
> +{
> +    destructStream();
> +}

This might be a sign that the stream should be a smart-pointer-like object that
cleans up when destroyed.  It also might not.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:96
> +void WebSocketTask::cancel()
> +{
> +
> +}
> +
> +void WebSocketTask::resume()
> +{
> +
> +}

Do these need to do anything?

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:119
> +	       handshakeMessage.remove(handshakeMessage.length() - 2, 2); //
Remove the terminating '\r\n'
> +	       handshakeMessage.append("Cookie: ");
> +	       handshakeMessage.append(cookieHeaderField);
> +	       handshakeMessage.append("\r\n\r\n");

This does 4 unnecessary string allocations and copies.	If handshakeMessage
were a StringBuilder or even a Vector<char> it would be more efficient.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:125
> +    auto buffer = makeUniqueArray<uint8_t>(handshakeData.length());
> +    memcpy(buffer.get(), handshakeData.data(), handshakeData.length());

This is another copy and allocation that might be able to be removed.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:156
> +    auto frameResult = receiveFrames([this](WebCore::WebSocketFrame::OpCode
opCode, const uint8_t* data, size_t length) {

It would probably be worth capturing a WeakPtr<WebSocketTask> and returning
early if it's null to be safe against using this after it has been freed.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:422
> +    callOnMainRunLoop([this, code, reason] {

Ditto, this may be deallocated by the time the lambda executes, which would be
bad.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.h:64
> +    enum class State {

nit
 : uint8_t