Summary: | WebSocket: Send ArrayBuffer 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, pimvdb, tkent, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 65249 | ||||||||
Attachments: |
|
Description
Yuta Kitamura
2011-09-02 05:05:32 PDT
Created attachment 106119 [details]
Patch
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. Created attachment 106248 [details]
Patch for landing
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 on attachment 106248 [details] Patch for landing Clearing flags on attachment: 106248 Committed r94482: <http://trac.webkit.org/changeset/94482> All reviewed patches have been landed. Closing bug. |