Bug 67477 - WebSocket: Send ArrayBuffer as WebSocket binary message
Summary: WebSocket: Send ArrayBuffer 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:
Blocks: 65249
  Show dependency treegraph
 
Reported: 2011-09-02 05:05 PDT by Yuta Kitamura
Modified: 2011-09-03 00:44 PDT (History)
5 users (show)

See Also:


Attachments
Patch (37.14 KB, patch)
2011-09-02 05:50 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch for landing (37.44 KB, patch)
2011-09-02 23:25 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-02 05:05:32 PDT
The last part of bug 65249, sibling of bug 67465.
Comment 1 Yuta Kitamura 2011-09-02 05:50:03 PDT
Created attachment 106119 [details]
Patch
Comment 2 Kent Tamura 2011-09-02 07:20:50 PDT
Comment on attachment 106119 [details]
Patch

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

> LayoutTests/http/tests/websocket/tests/hybi/workers/resources/send-blob.js:-5
> -function startsWith(target, prefix)
> -{
> -    return target.indexOf(prefix) === 0;
> -}
> -

ChangeLog should mention the reason of this removal.

> Source/WebCore/ChangeLog:22
> +        Rename "sent" to "sendResult" to clarify the meaning. Messages from the script may not be sent

nit:
Hmm, the sent -> sendResult renaming looks not to improve the readability. sendRequestResult?

> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:433
> +    OwnPtr<Vector<char> > data = adoptPtr(new Vector<char>);

We had better add a comment why we need to copy the ArrayBuffer to the Vector.

> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:436
> +    data->resize(binaryData.byteLength());
> +    if (binaryData.byteLength())
> +        memcpy(data->data(), binaryData.data(), binaryData.byteLength());

resize() should be reserveCapacity().
Your code initializes the vector buffer twice.
Comment 3 Yuta Kitamura 2011-09-02 23:25:30 PDT
Created attachment 106248 [details]
Patch for landing
Comment 4 Yuta Kitamura 2011-09-02 23:41:02 PDT
Comment on attachment 106119 [details]
Patch

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

Thank you for your comments!

>> LayoutTests/http/tests/websocket/tests/hybi/workers/resources/send-blob.js:-5
>> -
> 
> ChangeLog should mention the reason of this removal.

Added to ChangeLog.

>> Source/WebCore/ChangeLog:22
>> +        Rename "sent" to "sendResult" to clarify the meaning. Messages from the script may not be sent
> 
> nit:
> Hmm, the sent -> sendResult renaming looks not to improve the readability. sendRequestResult?

Renamed.

>> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:433
>> +    OwnPtr<Vector<char> > data = adoptPtr(new Vector<char>);
> 
> We had better add a comment why we need to copy the ArrayBuffer to the Vector.

Added a comment.

>> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:436
>> +        memcpy(data->data(), binaryData.data(), binaryData.byteLength());
> 
> resize() should be reserveCapacity().
> Your code initializes the vector buffer twice.

I don't think using reserveCapaity() is right, for two reasons:
- Initializing the vector elements will be turned into no-op, if the element type is primitive one. (VectorInitializer is doing some template magic)
- reserveCapacity() does not change the size of the vector, so we must add elements anyway. Appending each element should not be better than memcpy().

However, I realized resize() is not necessary becase we can use Vector constructor instead. I update the patch to do this.
Comment 5 WebKit Review Bot 2011-09-03 00:44:30 PDT
Comment on attachment 106248 [details]
Patch for landing

Clearing flags on attachment: 106248

Committed r94482: <http://trac.webkit.org/changeset/94482>
Comment 6 WebKit Review Bot 2011-09-03 00:44:35 PDT
All reviewed patches have been landed.  Closing bug.