Part of bug 65249.
Created attachment 106098 [details] Patch
Created attachment 106101 [details] Patch v2
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.
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?
Created attachment 106104 [details] Patch v3
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).
Comment on attachment 106104 [details] Patch v3 Oops, I missed last comments.
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()...
Created attachment 106107 [details] Patch v4
Created attachment 106108 [details] Patch v5
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.
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.
Comment on attachment 106108 [details] Patch v5 ok
Comment on attachment 106108 [details] Patch v5 Clearing flags on attachment: 106108 Committed r94399: <http://trac.webkit.org/changeset/94399>
All reviewed patches have been landed. Closing bug.
Committed r94405: <http://trac.webkit.org/changeset/94405>