[webkit-reviews] review denied: [Bug 75080] [Chromium] [WebSocket] Add WebArrayBuffer support in WebSocket to WebKit API : [Attachment 120307] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 22 09:19:14 PST 2011


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Takashi Toyoshima
<toyoshim at chromium.org>'s request for review:
Bug 75080: [Chromium] [WebSocket] Add WebArrayBuffer support in WebSocket to
WebKit API
https://bugs.webkit.org/show_bug.cgi?id=75080

Attachment 120307: Patch
https://bugs.webkit.org/attachment.cgi?id=120307&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.


More information about the webkit-reviews mailing list