[Webkit-unassigned] [Bug 199189] WebSockets: add support for sending blob messages when using web sockets platform APIs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=199189

--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 372915
  --> https://bugs.webkit.org/attachment.cgi?id=372915
Patch

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

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:229
>> +    CompletionHandler<void()> m_completionHandler;
> 
> Since these are completion handlers, we should call them when clearing the pending messages, otherwise there will be debug asserts.
> Maybe we should change the implementation so that keeping these completion handlers is not needed.
> That would mean slight adjustements to how we compute m_bufferedAmount.
> 
> Also this patch is doing multi blob loading in parallel.
> While this is better for performance, we might be able to simplify things by keeping the current behavior, which means having a Deque<Variant<String, ArrayBuffer, Blob>> and a bool m_loadingBlob.

There's a FIXME in current code, so I thought this was a good moment to fix that in the new code path.

// FIXME: Load two or more Blobs simultaneously for better performance.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:236
>> +        return send(JSC::ArrayBuffer::create(blob.size(), 1), 0, 0);
> 
> If size is greater than zero, we need to +/- m_bufferedAmount

Size is always 0, I'm using blob.size() instead of 0 directly to disambiguate from the different ::create() methods. This is calling send() anyway that already takes care of m_bufferedAmount, even though it will do nothing in this case.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:238
>> +    m_pendingMessages.append(std::make_unique<PendingMessage>(m_document.get(), blob, [this, protectedThis = makeRef(*this)] {
> 
> We might want to check whether the socket is closed there.
> A protectedThis is not needed here since m_pendingMessages is owned.

You mean checking it in the completion handler after the blob is loaded?

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:258
>> +                    send(*result, 0, result->byteLength());
> 
> We should probably do a sendWithAsyncReply here instead of a send.
> That might be needed for m_bufferedAmount and for readability.
> Initially I was puzzled of m_sendingPendingBlobMessage check in send(ArrayBuffer) but not send(String)

send() is needed for m_bufferedAmount, using sendWithAsyncReply here would require to define the completion handler here and handle buffered amount, that would duplicate code and make readability here worse for sure. I agree the m_sendingPendingBlobMessage is confusing though.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:260
>> +                    fail(makeString("Failed to load Blob: error code = ", errorCode.value()));
> 
> I wonder whether something more is needed, like sending an onerror message.
> Might be somehow testable with an Internals API to create a 'fake' blob.

Right, this is not correct, old code called WebSocketChannel::fail() which sends an onerror message, but in the new code path it sends Close message to the network process. What we want here is actually didReceiveMessageError(), or make fail() call m_client->didReceiveMessageError(); before sending the Close message.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190627/0be44c16/attachment.html>