Bug 92642 – [Bindings]Remove custom JS/V8 bindings for WebSocket::close() using [Clamp]

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

Created attachment 155278 [details]
patch_for_review

[Vineet Chaudhary (vineetc)]

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 :(

[Kentaro Hara]

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

[Kentaro Hara]

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

[Vineet Chaudhary (vineetc)]

(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
?

[Vineet Chaudhary (vineetc)]

(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);

[Kentaro Hara]

(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:)

[Vineet Chaudhary (vineetc)]

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

[Takashi Toyoshima]

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

[Takashi Toyoshima]

Ah, it's no problem to land this change without changing CloseEventCodeNotSpecified.

Vineet, thank you for supporting clamp and revise WebSocket to use it.

[WebKit Review Bot]

Comment on attachment 155278 [details]
patch_for_review

Clearing flags on attachment: 155278

Committed r124034: <http://trac.webkit.org/changeset/124034>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.