Bug 67477 – WebSocket: Send ArrayBuffer as WebSocket binary message

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

The last part of bug 65249, sibling of bug 67465.

[Yuta Kitamura]

Created attachment 106119 [details]
Patch

[Kent Tamura]

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.

[Yuta Kitamura]

Created attachment 106248 [details]
Patch for landing

[Yuta Kitamura]

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.

[WebKit Review Bot]

Comment on attachment 106248 [details]
Patch for landing

Clearing flags on attachment: 106248

Committed r94482: <http://trac.webkit.org/changeset/94482>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.