RESOLVED FIXED 75080
[Chromium] [WebSocket] Add WebArrayBuffer support in WebSocket to WebKit API
https://bugs.webkit.org/show_bug.cgi?id=75080
Summary [Chromium] [WebSocket] Add WebArrayBuffer support in WebSocket to WebKit API
Takashi Toyoshima
Reported 2011-12-22 04:45:30 PST
ArrayBuffer is exported as WebArrayBuffer. Now, WebSocket API support this WebArrayBuffer as one of binary data formats.
Attachments
Patch (7.87 KB, patch)
2011-12-22 04:55 PST, Takashi Toyoshima
no flags
Patch (7.95 KB, patch)
2011-12-26 02:46 PST, Takashi Toyoshima
no flags
Patch (7.92 KB, patch)
2011-12-26 02:58 PST, Takashi Toyoshima
no flags
Patch (7.95 KB, patch)
2011-12-26 21:01 PST, Takashi Toyoshima
no flags
Patch (9.07 KB, patch)
2012-01-10 00:51 PST, Takashi Toyoshima
no flags
Patch (9.05 KB, patch)
2012-01-11 21:38 PST, Takashi Toyoshima
no flags
Takashi Toyoshima
Comment 1 2011-12-22 04:55:52 PST
WebKit Review Bot
Comment 2 2011-12-22 04:57:41 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Darin Fisher (:fishd, Google)
Comment 3 2011-12-22 09:19:15 PST
Comment on attachment 120307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120307&action=review > Source/WebKit/chromium/src/WebSocketImpl.cpp:82 > + case BinaryTypeData: Why not just push the BinaryType enum into the WebKit API instead of having the embedder deal with strings? class WebSocket { ... enum BinaryType { ... }; Is "BinaryType" the best name for this field? XHR calls it "responseType"... you could also perhaps go with "DataType"... hmm... Also, it looks like BinaryType only impacts how data is received. It looks like you can say that you only receive ArrayBuffers but then you can send text. Is that flexibility intended? > Source/WebKit/chromium/src/WebSocketImpl.cpp:149 > + WTF::PassRefPtr<WTF::ArrayBuffer> arrayBuffer = webArrayBuffer; nit: no need for WTF:: prefix in .cpp files.
Takashi Toyoshima
Comment 4 2011-12-22 17:58:16 PST
The WebSocket API ( http://dev.w3.org/html5/websockets/ ) defines the binaryType property. And my implementation come from the spec. For example, readyState is defined as enum in the spec, so I just map it as enum by using spec defining enum numbers like CONNECTING(0), OPEN(1), .... On the other hands, binaryType is defined as String. If we make this interface using internal enum directly, its actual id value is not defined in the spec, but depends on our implementation. Anyway, type of binaryType could be enum here, I think. About its behaviors, WebSocket protocol defines text frames and binary frames. They represent sending and receiving packets. Text frames always map to DOMString, but binary frames could be mapped to used chosen binary object like ArrayBuffer, and Blob. This attribute is for specifying its receiving binary object type. We can always send all data types, DOMString, ArrayBuffer, and Blob. But receiving type must be DOMString and ArrayBuffer, or DOMString and Blob. XHR's receiving data doesn't have data types originally, but WebSocket data has. This is the reason why XHR defines responseType, and WebSocket defines binaryType, I think. So, I agree with making its type enum. But I still want to know your opinions on its property name and its function itself. Thanks,
Darin Fisher (:fishd, Google)
Comment 5 2011-12-22 21:03:28 PST
+1 for using an enum instead of a string. +1 for sticking with "BinaryType" since that comes from the spec. +1 for BinaryType selection only impacting how data is received. Please just add a comment above the binaryType attribute explaining what it means and please also define what the default value is.
Takashi Toyoshima
Comment 6 2011-12-26 02:46:30 PST
Takashi Toyoshima
Comment 7 2011-12-26 02:50:43 PST
Happy holidays! Thank you for your advice. I fixed my change to use enum and to have comments.
Takashi Toyoshima
Comment 8 2011-12-26 02:58:56 PST
Takashi Toyoshima
Comment 9 2011-12-26 02:59:43 PST
Sorry, I missed to fix WTF prefix nits. I revised again.
Takashi Toyoshima
Comment 10 2011-12-26 21:01:12 PST
Takashi Toyoshima
Comment 11 2011-12-26 21:03:38 PST
I clarified enum-ed values for BinaryType and reorder them because I'd like to define fixed number 0 and 1 to Blob and ArrayBuffer. These value will be also used in pepper's IDL definition.
Takashi Toyoshima
Comment 12 2012-01-10 00:51:03 PST
Takashi Toyoshima
Comment 13 2012-01-10 00:54:00 PST
A happy new year! Just rebased to the trunk and updated copyrights' year. Thanks
Darin Fisher (:fishd, Google)
Comment 14 2012-01-11 21:10:18 PST
Comment on attachment 121807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121807&action=review > Source/WebKit/chromium/src/WebSocketImpl.cpp:131 > + return m_private->send(*static_cast<PassRefPtr<ArrayBuffer> >(webArrayBuffer)); nit: we usually use PassRefPtr<ArrayBuffer>(webArrayBuffer) in cases like this. i.e., construct a PassRefPtr. of course, the compiler will invoke the casting operator, and as such it'll result in the same code you have here. it just looks a bit cleaner as a call to the PassRefPtr constructor.
Takashi Toyoshima
Comment 15 2012-01-11 21:36:30 PST
Comment on attachment 121807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121807&action=review >> Source/WebKit/chromium/src/WebSocketImpl.cpp:131 >> + return m_private->send(*static_cast<PassRefPtr<ArrayBuffer> >(webArrayBuffer)); > > nit: we usually use PassRefPtr<ArrayBuffer>(webArrayBuffer) in cases like this. > i.e., construct a PassRefPtr. of course, the compiler will invoke the casting > operator, and as such it'll result in the same code you have here. it just > looks a bit cleaner as a call to the PassRefPtr constructor. Thanks. I fixed it.
Takashi Toyoshima
Comment 16 2012-01-11 21:38:42 PST
WebKit Review Bot
Comment 17 2012-01-11 22:48:00 PST
Comment on attachment 122166 [details] Patch Clearing flags on attachment: 122166 Committed r104791: <http://trac.webkit.org/changeset/104791>
WebKit Review Bot
Comment 18 2012-01-11 22:48:06 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.