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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 26 22:45:52 PDT 2019


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

--- Comment #5 from youenn fablet <youennf at gmail.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:159
> +    const RefPtr<JSC::ArrayBuffer>& result() const { return m_buffer; }

Preexisting issue in WebSocket code so I guess this is fine.
But it does not seem super great to get a JSC::ArrayBuffer.
Maybe we should do some refactoring or use FetchLoader like in FetchBodyOwner.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:195
> +    PendingMessage(const String& message, CompletionHandler<void()>&& completionHandler)

String&&

> 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.

> 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

> 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.

> 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)

> 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.

-- 
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/32994756/attachment-0001.html>


More information about the webkit-unassigned mailing list