WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(7.95 KB, patch)
2011-12-26 02:46 PST
,
Takashi Toyoshima
no flags
Details
Formatted Diff
Diff
Patch
(7.92 KB, patch)
2011-12-26 02:58 PST
,
Takashi Toyoshima
no flags
Details
Formatted Diff
Diff
Patch
(7.95 KB, patch)
2011-12-26 21:01 PST
,
Takashi Toyoshima
no flags
Details
Formatted Diff
Diff
Patch
(9.07 KB, patch)
2012-01-10 00:51 PST
,
Takashi Toyoshima
no flags
Details
Formatted Diff
Diff
Patch
(9.05 KB, patch)
2012-01-11 21:38 PST
,
Takashi Toyoshima
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Takashi Toyoshima
Comment 1
2011-12-22 04:55:52 PST
Created
attachment 120307
[details]
Patch
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
Created
attachment 120542
[details]
Patch
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
Created
attachment 120543
[details]
Patch
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
Created
attachment 120563
[details]
Patch
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
Created
attachment 121807
[details]
Patch
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
Created
attachment 122166
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug