Bug 75080 – [Chromium] [WebSocket] Add WebArrayBuffer support in WebSocket to WebKit API

• Component: WebKit API
• Status: RESOLVED FIXED

[Takashi Toyoshima]

ArrayBuffer is exported as WebArrayBuffer.
Now, WebSocket API support this WebArrayBuffer as one of binary data formats.

[Takashi Toyoshima]

Created attachment 120307 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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]

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)]

+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]

Created attachment 120542 [details]
Patch

[Takashi Toyoshima]

Happy holidays!

Thank you for your advice.
I fixed my change to use enum and to have comments.

[Takashi Toyoshima]

Created attachment 120543 [details]
Patch

[Takashi Toyoshima]

Sorry, I missed to fix WTF prefix nits.
I revised again.

[Takashi Toyoshima]

Created attachment 120563 [details]
Patch

[Takashi Toyoshima]

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]

Created attachment 121807 [details]
Patch

[Takashi Toyoshima]

A happy new year!

Just rebased to the trunk and updated copyrights' year.
Thanks

[Darin Fisher (:fishd, Google)]

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 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]

Created attachment 122166 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122166 [details]
Patch

Clearing flags on attachment: 122166

Committed r104791: <http://trac.webkit.org/changeset/104791>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.