[webkit-reviews] review granted: [Bug 199189] WebSockets: add support for sending blob messages when using web sockets platform APIs : [Attachment 373095] Try to fix apple builds

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 199189: WebSockets: add support for sending blob messages when using web
sockets platform APIs
https://bugs.webkit.org/show_bug.cgi?id=199189

Attachment 373095: Try to fix apple builds

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




--- Comment #12 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 373095
  --> https://bugs.webkit.org/attachment.cgi?id=373095
Try to fix apple builds

r=me.

Since we might want the same mechanism for DataChannel, I wonder though whether
we should not have a more generic mechanism.
Something like a Queue where we can push String/ArrayBuffer/Blob.
The queue could be query whether empty or not.
It would take a Function callback to notify that a Variant<String, ArrayBuffer>
is ready to be sent.

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

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:253
> +

As per spec, I think we should increase the buffered amount by blob.size()
synchronously.
And decrement it when receiving the IPC message.
This does not seem to be like this in the previous implementation though.
It might be nice to have a test for that and check what other browsers are
doing.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:264
> +		   sendMessage(Messages::NetworkSocketChannel::SendData {
IPC::DataReference { reinterpret_cast<const uint8_t*>(binaryData.data()),
binaryData.size() } }, binaryData.size());

Maybe we should have routines for these sendMessage(text) sendMessage(binary).

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:277
> +		       fail(makeString("Failed to load Blob: error code = ",
errorCode.value()));

I am not sure this error handling is good enough.
If there is no test coverage for that case, we should file a bug.