Bug 66298

Summary:

WebSocket: Queue messages to be sent

Product:

WebKit

Reporter:

Yuta Kitamura <yutak>

Component:

WebCore Misc.

Assignee:

Yuta Kitamura <yutak>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

65249

Attachments:

Description	Flags
Patch	none
Patch v2	none
Patch v3	none

Yuta Kitamura

Reported 2011-08-16 06:48:18 PDT

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.

Attachments
Patch (10.94 KB, patch) 2011-08-16 07:14 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Patch v2 (11.46 KB, patch) 2011-08-21 19:39 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Patch v3 (11.45 KB, patch) 2011-08-25 03:10 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yuta Kitamura

Comment 1 2011-08-16 07:14:04 PDT

Created attachment 104038 [details] Patch

Kent Tamura

Comment 2 2011-08-18 00:57:34 PDT

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

Comment 3 2011-08-21 19:39:04 PDT

Created attachment 104637 [details] Patch v2

Yuta Kitamura

Comment 4 2011-08-21 19:40:51 PDT

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 5 2011-08-21 20:04:19 PDT

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

Comment 6 2011-08-25 03:10:47 PDT

Created attachment 105155 [details] Patch v3

Kent Tamura

Comment 7 2011-08-25 03:16:36 PDT

Comment on attachment 105155 [details] Patch v3 ok

Yuta Kitamura

Comment 8 2011-08-25 03:17:30 PDT

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 9 2011-08-25 04:32:00 PDT

Comment on attachment 105155 [details] Patch v3 Clearing flags on attachment: 105155 Committed r93773: <http://trac.webkit.org/changeset/93773>

WebKit Review Bot

Comment 10 2011-08-25 04:32:05 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.