Bug 67465 - WebSocket: Send Blob as WebSocket binary message
Summary: WebSocket: Send Blob as WebSocket binary message
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on: 67471
Blocks: 65249
  Show dependency treegraph
 
Reported: 2011-09-01 22:49 PDT by Yuta Kitamura
Modified: 2011-09-02 04:15 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-09-01 22:49:34 PDT
Part of bug 65249.
Comment 1 Yuta Kitamura 2011-09-01 23:19:42 PDT
Created attachment 106098 [details]
Patch
Comment 2 Yuta Kitamura 2011-09-02 00:29:25 PDT
Created attachment 106101 [details]
Patch v2
Comment 3 Kent Tamura 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.
Comment 4 Kent Tamura 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?
Comment 5 Yuta Kitamura 2011-09-02 01:23:14 PDT
Created attachment 106104 [details]
Patch v3
Comment 6 Yuta Kitamura 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).
Comment 7 Yuta Kitamura 2011-09-02 01:26:32 PDT
Comment on attachment 106104 [details]
Patch v3

Oops, I missed last comments.
Comment 8 Yuta Kitamura 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()...
Comment 9 Yuta Kitamura 2011-09-02 02:23:25 PDT
Created attachment 106107 [details]
Patch v4
Comment 10 Yuta Kitamura 2011-09-02 02:26:28 PDT
Created attachment 106108 [details]
Patch v5
Comment 11 Yuta Kitamura 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.
Comment 12 Kent Tamura 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.
Comment 13 Kent Tamura 2011-09-02 02:34:00 PDT
Comment on attachment 106108 [details]
Patch v5

ok
Comment 14 WebKit Review Bot 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>
Comment 15 WebKit Review Bot 2011-09-02 03:35:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Yuta Kitamura 2011-09-02 04:15:46 PDT
Committed r94405: <http://trac.webkit.org/changeset/94405>