Bug 67465

Summary:

WebSocket: Send Blob as WebSocket binary message

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:

67471

Bug Blocks:

65249

Attachments:

Description	Flags
Patch	none
Patch v2	none
Patch v3	none
Patch v4	none
Patch v5	none

Yuta Kitamura

Reported 2011-09-01 22:49:34 PDT

Part of bug 65249.

Attachments
Patch (37.97 KB, patch) 2011-09-01 23:19 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Patch v2 (38.05 KB, patch) 2011-09-02 00:29 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Patch v3 (41.50 KB, patch) 2011-09-02 01:23 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Patch v4 (49.98 KB, patch) 2011-09-02 02:23 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Patch v5 (49.97 KB, patch) 2011-09-02 02:26 PDT, Yuta Kitamura	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Yuta Kitamura

Comment 1 2011-09-01 23:19:42 PDT

Created attachment 106098 [details] Patch

Yuta Kitamura

Comment 2 2011-09-02 00:29:25 PDT

Created attachment 106101 [details] Patch v2

Kent Tamura

Comment 3 2011-09-02 00:36:33 PDT

Comment on attachment 106098 [details] Patch r- because of test coverage. View in context: https://bugs.webkit.org/attachment.cgi?id=106098&action=review > Source/WebCore/bindings/js/JSWebSocketCustom.cpp:92 > + if (!exec->argumentCount()) > + return throwError(exec, createSyntaxError(exec, "Not enough arguments")); Do we have a test for this? > Source/WebCore/websockets/ThreadableWebSocketChannel.h:57 > + virtual bool send(const Blob& binaryData) = 0; "binaryData" argument name should be removed. > Source/WebCore/websockets/WebSocket.cpp:276 > + if (m_state == CLOSING || m_state == CLOSED) { > + m_bufferedAmountAfterClose += binaryData->size() + getFramingOverhead(binaryData->size()); > + return false; No test for this block. We don't throw an exception, right? > Source/WebCore/websockets/WebSocket.h:71 > + bool send(Blob* binaryData, ExceptionCode&); should remove "binaryData" argument name. > Source/WebCore/websockets/WebSocketChannel.h:69 > + virtual bool send(const Blob& binaryData); Should remove "binaryData" argument name > Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:189 > + bool sent = m_mainWebSocketChannel->send(binaryData); > + m_loaderProxy.postTaskForModeToWorkerContext(createCallbackTask(&workerContextDidSend, m_workerClientWrapper, sent), m_taskMode); 'sent' is confusing. It means 'enqueued', right? > Source/WebCore/websockets/WorkerThreadableWebSocketChannel.h:67 > + virtual bool send(const Blob& binaryData); Should remove 'binaryData' argument name. > Source/WebCore/websockets/WorkerThreadableWebSocketChannel.h:97 > + void send(const Blob& binaryData); ditto. > Source/WebCore/websockets/WorkerThreadableWebSocketChannel.h:130 > + bool send(const Blob& binaryData); ditto.

Kent Tamura

Comment 4 2011-09-02 00:39:26 PDT

Comment on attachment 106101 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=106101&action=review r- because of conflicts. > Source/WebCore/websockets/WebSocket.cpp:271 > + if (m_useHixie76Protocol) > + return send("[object Blob]", ec); Is there a test for this?

Yuta Kitamura

Comment 5 2011-09-02 01:23:14 PDT

Created attachment 106104 [details] Patch v3

Yuta Kitamura

Comment 6 2011-09-02 01:24:11 PDT

Comment on attachment 106101 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=106101&action=review >> Source/WebCore/websockets/WebSocket.cpp:271 >> + return send("[object Blob]", ec); > > Is there a test for this? Added a test (hixie76/send-object.html).

Yuta Kitamura

Comment 7 2011-09-02 01:26:32 PDT

Comment on attachment 106104 [details] Patch v3 Oops, I missed last comments.

Yuta Kitamura

Comment 8 2011-09-02 02:14:52 PDT

Comment on attachment 106098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106098&action=review >> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:189 >> + m_loaderProxy.postTaskForModeToWorkerContext(createCallbackTask(&workerContextDidSend, m_workerClientWrapper, sent), m_taskMode); > > 'sent' is confusing. It means 'enqueued', right? The word "sent" here (and also in ThreadableWebSocketChannelClientWrapper.h) is used as "the return value of send()", so using the term "enqueued" seems more confusing, as it does not sound like the variable is related to send()...

Yuta Kitamura

Comment 9 2011-09-02 02:23:25 PDT

Created attachment 106107 [details] Patch v4

Yuta Kitamura

Comment 10 2011-09-02 02:26:28 PDT

Created attachment 106108 [details] Patch v5

Yuta Kitamura

Comment 11 2011-09-02 02:27:25 PDT

Comment on attachment 106098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106098&action=review >> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:92 >> + return throwError(exec, createSyntaxError(exec, "Not enough arguments")); > > Do we have a test for this? Added a test (hixie76/send-empty.html, hybi/send-empty.html). >> Source/WebCore/websockets/ThreadableWebSocketChannel.h:57 >> + virtual bool send(const Blob& binaryData) = 0; > > "binaryData" argument name should be removed. Removed. >> Source/WebCore/websockets/WebSocket.cpp:276 >> + return false; > > No test for this block. > We don't throw an exception, right? Updated hybi/bufferedAmount-after-close.html. >> Source/WebCore/websockets/WebSocket.h:71 >> + bool send(Blob* binaryData, ExceptionCode&); > > should remove "binaryData" argument name. Removed. >> Source/WebCore/websockets/WebSocketChannel.h:69 >> + virtual bool send(const Blob& binaryData); > > Should remove "binaryData" argument name Removed. >> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.h:67 >> + virtual bool send(const Blob& binaryData); > > Should remove 'binaryData' argument name. Removed. >> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.h:97 >> + void send(const Blob& binaryData); > > ditto. Removed. >> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.h:130 >> + bool send(const Blob& binaryData); > > ditto. Removed.

Kent Tamura

Comment 12 2011-09-02 02:33:00 PDT

Comment on attachment 106098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106098&action=review >>> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:189 >>> + m_loaderProxy.postTaskForModeToWorkerContext(createCallbackTask(&workerContextDidSend, m_workerClientWrapper, sent), m_taskMode); >> >> 'sent' is confusing. It means 'enqueued', right? > > The word "sent" here (and also in ThreadableWebSocketChannelClientWrapper.h) is used as "the return value of send()", so using the term "enqueued" seems more confusing, as it does not sound like the variable is related to send()... So "sendResult" would be a candidate. We can rename WebSocketChannel::send() to another name, though we can't rename WebSocket::send(). You and I know send(Blob) is asynchronous. However other developers who read websocket code in the future will be confused by it.

Kent Tamura

Comment 13 2011-09-02 02:34:00 PDT

Comment on attachment 106108 [details] Patch v5 ok

WebKit Review Bot

Comment 14 2011-09-02 03:35:51 PDT

Comment on attachment 106108 [details] Patch v5 Clearing flags on attachment: 106108 Committed r94399: <http://trac.webkit.org/changeset/94399>

WebKit Review Bot

Comment 15 2011-09-02 03:35:57 PDT

All reviewed patches have been landed. Closing bug.

Yuta Kitamura

Comment 16 2011-09-02 04:15:46 PDT

Committed r94405: <http://trac.webkit.org/changeset/94405>

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