[webkit-reviews] review denied: [Bug 172630] [Curl] Add support for wss:// websockets : [Attachment 345476] FIX

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 172630: [Curl] Add support for wss:// websockets
https://bugs.webkit.org/show_bug.cgi?id=172630

Attachment 345476: FIX

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




--- Comment #23 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 345476
  --> https://bugs.webkit.org/attachment.cgi?id=345476
FIX

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

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:85
> +    std::array<uint8_t, kLocalSendBufferSize> m_writeBuffer;

This consumes 16KiB for easy WebSocket.
There are still two outgoing buffers m_buffer and m_writeBuffer.
Now, I understand the reason why curl port needs two outgoing buffers while
soup and mac ports don't, because curl port is sending data in the worker
thread.
This is completely my fault. Your original implementation doesn't look bad. orz

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:90
> +    memcpy(&m_writeBuffer[0], data, length);

Did you forget fix this? Or any problem?

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:127
> +    const size_t BufferSize = 1024;

You renamed 'bufferSize' to 'BufferSize'. I think this doesn't comply with the
WebKit naming convention. Should be 'bufferSize', 'kBufferSize', or
'cBufferSize'.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:139
> +	       auto data = &m_writeBuffer[0] + state.offset;

Did you forget fix this? Or any problem?

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:180
> +    callOnMainThread([this, protectedThis = makeRef(*this), errorCode,
localizedDescription = CurlHandle::errorDescription(errorCode)] {

Did you forget fix this? Or any problem?