Bug 66298 – WebSocket: Queue messages to be sent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yuta Kitamura]

To send a Blob from WebSocket, we must read it asynchronously. Therefore, a Blob cannot be sent immediately, and other data types (String or ArrayBuffer) may also be blocked by other pending sending requests.

To get this behavior correctly, we need to create a queue of WebSocket messages to be sent, and send them in the order they arrived.

[Yuta Kitamura]

Created attachment 104038 [details]
Patch

[Kent Tamura]

Comment on attachment 104038 [details]
Patch

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

I feel the names don't look perfect.
IMO,
  SendItem -> QueuedFrame
  addStringToSendQueue() -> enqueueStringFrame()
  addRawDataToSendQueue() -> enqueueRawFrame()
  SendQueue -> OutgoingFrameQueue or SendingFrameQueue

What do you think?

> Source/WebCore/websockets/WebSocketChannel.cpp:763
> +    m_sendQueue.append(item.release());
> +}

Can you call processSendQueue() here instead of calling processSendQueue() in call sites?

> Source/WebCore/websockets/WebSocketChannel.cpp:776
> +    m_sendQueue.append(item.release());
> +}

ditto.

[Yuta Kitamura]

Created attachment 104637 [details]
Patch v2

[Yuta Kitamura]

Comment on attachment 104038 [details]
Patch

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

In Patch v2, names are updated as you suggested.

>> Source/WebCore/websockets/WebSocketChannel.cpp:763
>> +}
> 
> Can you call processSendQueue() here instead of calling processSendQueue() in call sites?

Fixed in patch v2.

>> Source/WebCore/websockets/WebSocketChannel.cpp:776
>> +}
> 
> ditto.

Fixed too.

[Kent Tamura]

Comment on attachment 104637 [details]
Patch v2

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

> Source/WebCore/websockets/WebSocketChannel.cpp:759
> +void WebSocketChannel::enqueueStringFrame(OpCode opCode, const String& string)

Is it possible to call enqueueStringFrame with an OpCode other than OpCodeText?
If not, we can omit OpCode argument.

[Yuta Kitamura]

Created attachment 105155 [details]
Patch v3

[Kent Tamura]

Comment on attachment 105155 [details]
Patch v3

ok

[Yuta Kitamura]

Comment on attachment 104637 [details]
Patch v2

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

>> Source/WebCore/websockets/WebSocketChannel.cpp:759
>> +void WebSocketChannel::enqueueStringFrame(OpCode opCode, const String& string)
> 
> Is it possible to call enqueueStringFrame with an OpCode other than OpCodeText?
> If not, we can omit OpCode argument.

OpCode argument has been removed, and this member function has been renamed to euqueueTextFrame so it clearly shows that a text frame is going to be sent.

[WebKit Review Bot]

Comment on attachment 105155 [details]
Patch v3

Clearing flags on attachment: 105155

Committed r93773: <http://trac.webkit.org/changeset/93773>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.