Bug 201359 – Abstract out WebSocketChannel message queue

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

This can be used by RTCDataChannel as well when implementing send(Blob)

[youenn fablet]

Created attachment 377757 [details]
Patch

[Alex Christensen]

Comment on attachment 377757 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        (WebCore::MessageQueueWithBlobs::MessageQueueWithBlobs):

I think this needs a better name, like NetworkSendQueue or something.  This currently looks related to WTF::MessageQueue, but it's not.

> Source/WebCore/fileapi/MessageQueueWithBlobs.h:46
> +    MessageQueueWithBlobs(Document&, Function<void(const String&)>&&, Function<void(const char*, size_t)>&&, Function<bool(int)>&&);

I think these functions need names in the header.  Why does notifyError return a bool?

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:56
> +    }, [&channel](const char* data, size_t byteLength) {
> +        channel.sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(data), byteLength } }, byteLength);

This captures a raw pointer and uses it later.  I think we should capture a strong or weak reference instead.

[youenn fablet]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 377757 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377757&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        (WebCore::MessageQueueWithBlobs::MessageQueueWithBlobs):
> 
> I think this needs a better name, like NetworkSendQueue or something.  This
> currently looks related to WTF::MessageQueue, but it's not.

OK.
 
> > Source/WebCore/fileapi/MessageQueueWithBlobs.h:46
> > +    MessageQueueWithBlobs(Document&, Function<void(const String&)>&&, Function<void(const char*, size_t)>&&, Function<bool(int)>&&);
> 
> I think these functions need names in the header.  Why does notifyError
> return a bool?

Will add names, notifyError returns a boolean to determine whether processing messages should stop after an error or not.
I will change it to processError.

> > Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:56
> > +    }, [&channel](const char* data, size_t byteLength) {
> > +        channel.sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(data), byteLength } }, byteLength);
> 
> This captures a raw pointer and uses it later.  I think we should capture a
> strong or weak reference instead.

channel is owning the MessageQueueWithBlobs so the lambdas will be destroyed at channel destruction time.
The only tricky part is the case of the lambdas being called during the destruction of channel.
I believe this is handled by the ABORT_ERROR check.

[youenn fablet]

Created attachment 377788 [details]
Patch

[youenn fablet]

Created attachment 377796 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377796 [details]
Patch

Clearing flags on attachment: 377796

Committed r249481: <https://trac.webkit.org/changeset/249481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55023788>