With support of [Clamp] as extended attribute we can replace custom bindings from WebSocket::close(). Spec for WebSocket::close() : http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#websocket
Created attachment 155278 [details] patch_for_review
Comment on attachment 155278 [details] patch_for_review View in context: https://bugs.webkit.org/attachment.cgi?id=155278&action=review > Source/WebCore/ChangeLog:22 > + (GenerateParametersCheck): I apologize for this, its regression from my previous patch :(
Comment on attachment 155278 [details] patch_for_review View in context: https://bugs.webkit.org/attachment.cgi?id=155278&action=review LGTM, this is a great progress. > Source/WebCore/Modules/websockets/WebSocket.idl:76 > + void close(in [Clamp, Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException); BTW, isn't it allowed to change the value of WebSocketChannel::CloseEventCodeNotSpecified from -1 to 0? If we can change it to 0, we can make the binding code clearer: by specifying [Optional=DefaultIsUndefined] for 'code' and [Optional=DefaultIsNullString] for 'reason', we can remove close(ExceptionCode& ec) and close(int code, ExceptionCode& ec).
Comment on attachment 155278 [details] patch_for_review > BTW, isn't it allowed to change the value of WebSocketChannel::CloseEventCodeNotSpecified from -1 to 0? Sorry, I was misunderstanding. Please ignore the comment:)
(In reply to comment #4) > (From update of attachment 155278 [details]) > > BTW, isn't it allowed to change the value of WebSocketChannel::CloseEventCodeNotSpecified from -1 to 0? > > Sorry, I was misunderstanding. Please ignore the comment:) > BTW, isn't it allowed to change the value of WebSocketChannel::CloseEventCodeNotSpecified from -1 to 0? > > If we can change it to 0, we can make the binding code clearer: by specifying [Optional=DefaultIsUndefined] for 'code' and [Optional=DefaultIsNullString] for 'reason', we can remove close(ExceptionCode& ec) and close(int code, ExceptionCode& ec). As per the http://tools.ietf.org/html/rfc6455#section-7.4.1 we can change "-1" to "0" as 0-999 are not in used. But I am really not sure if this doesn't breaks existing behaviour of chrome as it might be based on this code(websocket.h). Shall we check with chromium peoples ?
(In reply to comment #5) > As per the http://tools.ietf.org/html/rfc6455#section-7.4.1 we can change "-1" to "0" as 0-999 are not in used. > But I am really not sure if this doesn't breaks existing behaviour of chrome as it might be based on this code(websocket.h). Shall we check with chromium peoples > ? Rather we should use 0 than -1 as the spec itself says it should be unsigned void close([Clamp] optional unsigned short code, optional DOMString reason);
(In reply to comment #6) > (In reply to comment #5) > > As per the http://tools.ietf.org/html/rfc6455#section-7.4.1 we can change "-1" to "0" as 0-999 are not in used. > > But I am really not sure if this doesn't breaks existing behaviour of chrome as it might be based on this code(websocket.h). Shall we check with chromium peoples > > ? > > Rather we should use 0 than -1 as the spec itself says it should be unsigned > > void close([Clamp] optional unsigned short code, optional DOMString reason); Oh, good point. toyoshim: Do you have any concern about changing the default 'code' value from -1 to 0? vineetc: Either way let's fix the problem in a follow-up patch:)
Comment on attachment 155278 [details] patch_for_review > vineetc: Either way let's fix the problem in a follow-up patch:) Oke we will address/discuss this in another thread. Thanks..
> toyoshim: Do you have any concern about changing the default 'code' value from -1 to 0? In WebSocket::close(), code and reason are defined as "optional". Originally, this out of range value -1 is used to detect the case there values are omitted. Another concern is that this value is exported via WebKit API. $ git grep CloseEventCodeNotSpecified (snip) WebKit/chromium/public/WebSocket.h: CloseEventCodeNotSpecified = -1, This value is used in chromium side to support PPAPI bindings. Changing this value will break some chromium unit tests temporally, but I can fix them in chromium side. Anyway, I should disable these tests temporally, so please ping me before landing this change.
Ah, it's no problem to land this change without changing CloseEventCodeNotSpecified. Vineet, thank you for supporting clamp and revise WebSocket to use it.
Comment on attachment 155278 [details] patch_for_review Clearing flags on attachment: 155278 Committed r124034: <http://trac.webkit.org/changeset/124034>
All reviewed patches have been landed. Closing bug.