Bug 66925

Created attachment 105146 [details] Patch (v8 code missing now)

Created attachment 105176 [details] Patch (include pywebsocket update patch)

Created attachment 105178 [details] Patch

Second patch included 66924 change. Difference between second one and third one is that.

Comment on attachment 105178 [details] Patch Would you post a patch which won't cause conflicts please?

Created attachment 105338 [details] Patch

Comment on attachment 105338 [details] Patch Attachment 105338 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9508916

Created attachment 105340 [details] Patch

Comment on attachment 105340 [details] Patch Attachment 105340 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9508927 New failing tests: http/tests/websocket/tests/hybi/client-close.html http/tests/websocket/tests/hybi/close.html

Comment on attachment 105340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105340&action=review > LayoutTests/http/tests/websocket/tests/hybi/close.html:108 > + // TODO(toyoshim): unpaired surrogates throw SYNTAX_ERR TODO(name) is not a WebKit style. > LayoutTests/http/tests/websocket/tests/hybi/close_wsh.py:56 > + > + > +# vi:sts=4 sw=4 et Should remove such editor-specific annotation. > LayoutTests/http/tests/websocket/tests/hybi/workers/resources/close.js:120 > + //shouldBeTrue("exceptionProto === DOMException.prototype"); should remove unused code. > LayoutTests/http/tests/websocket/tests/hybi/workers/resources/close.js:160 > + //shouldBeTrue("exceptionProto === DOMException.prototype"); ditto. > Source/WebCore/bindings/js/JSWebSocketCustom.cpp:93 > + int argumentCount = static_cast<int>(exec->argumentCount()); Why don't you use unsigned or size_t for argumentCount? > Source/WebCore/websockets/WebSocketChannel.cpp:737 > + startClosingHandshake(0, ""); What does "0" mean?

Comment on attachment 105340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105340&action=review > Source/WebCore/bindings/js/JSWebSocketCustom.cpp:102 > + code = clampToInteger(x); Just curious, what happens if isnan(x) is true? > Source/WebCore/bindings/js/JSWebSocketCustom.cpp:105 > + reason = ustringToString(exec->argument(1).toString(exec)); toString() may raise an exception. If that happens, you need to exit the function (see constructJSWebSocket() function above). > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:121 > + double x = args[0]->NumberValue(); Lousy indentation here. > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:123 > + x = clampTo(x, 0, 0xffff); You could use USHORT_MAX, or numeric_limit<uint16_t>::max(). > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:127 > + reason = toWebCoreString(args[1]->ToString()); Same here, you need to watch an exception coming from ToString(). > Source/WebCore/websockets/WebSocket.cpp:267 > + if (!(code == WebSocketChannel::CloseEventCodeNormalClosure || (3000 <= code && code <= 4999))) { You probably need to give names to bare numbers like 3000 and 4999. > Source/WebCore/websockets/WebSocket.cpp:272 > + if (reason.utf8().length() > 123) { Same here, define a constant for the value 123.

Comment on attachment 105340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105340&action=review >> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:102 >> + code = clampToInteger(x); > > Just curious, what happens if isnan(x) is true? clampToInteger handle NaN by static cast to int. I guess its result depends on compilers. I assign 0.0 explicitly when x is NaN. >> Source/WebCore/websockets/WebSocketChannel.cpp:737 >> + startClosingHandshake(0, ""); > > What does "0" mean? Actually, these arguments have no mean because this function is for old protocol hixie76. Hixie76 doesn't use code and reason. Anyway, I use CloseEventCodeNotSpecified (-1) for readability.

Created attachment 105763 [details] Patch

Created attachment 105767 [details] Patch

Comment on attachment 105767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105767&action=review r- for some style issues. > LayoutTests/http/tests/websocket/tests/hybi/close-expected.txt:7 > +invalid code test: 0 nit: Capitalize: invalid -> Invalid > LayoutTests/http/tests/websocket/tests/hybi/close-expected.txt:8 > +code 999 must cause INVALID_ACCESS_ERR nit: This is a sentence. So "code" should be "Code", and should end with '.'. > LayoutTests/http/tests/websocket/tests/hybi/close.html:48 > +var onopen = function () The names onopen, onerror, onclose, onmessage are confusing. I recommend to rename them like handleOpen, handleError, .... > LayoutTests/http/tests/websocket/tests/hybi/close.html:68 > +var set_defaults = function (ws) We usually use the naming rule same as C++. So this should be "setDefaults". Also, there is no reason to use "var". function setDefaultHandlers(ws) { .... > LayoutTests/http/tests/websocket/tests/hybi/close.html:76 > +var run_code_test = function () ditto. function runCodeTest() ... > LayoutTests/http/tests/websocket/tests/hybi/close.html:106 > +var run_invalid_string_test = function () ditto. function runInvalidStringTest() ... > LayoutTests/http/tests/websocket/tests/hybi/close.html:113 > +var run_reason_test = function (next_test) ditto. function runReasonTest() ... > Source/WebCore/bindings/js/JSWebSocketCustom.cpp:109 > + // in optional DOMString reason It seems that this comment adds no information to the code. It should be removed. > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:131 > + // in optional DOMString reason ditto.

Created attachment 105909 [details] Patch

Comment on attachment 105909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105909&action=review > LayoutTests/http/tests/websocket/tests/hybi/close.html:108 > + // TODO: unpaired surrogates throw SYNTAX_ERR TODO -> FIXME > Source/WebCore/websockets/WebSocket.idl:67 > - void close(); > + [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException); Do we really need custom binding code? Probably we can avoid [Custom] by providing close(ExceptionCode&), close(unsigned short, ExceptionCode&), and close(unsigned short, const String&, ExceptionCode&) in WebSocket.

Comment on attachment 105909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105909&action=review >> Source/WebCore/websockets/WebSocket.idl:67 >> + [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException); > > Do we really need custom binding code? > Probably we can avoid [Custom] by providing close(ExceptionCode&), close(unsigned short, ExceptionCode&), and close(unsigned short, const String&, ExceptionCode&) in WebSocket. Main purpose to use [Custom] is to realize [Clamp] attribute behavior exactly. Autogenerated code will handle casting from Number to unsigned short as modulo.

Created attachment 105915 [details] Patch

Comment on attachment 105909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105909&action=review >>> Source/WebCore/websockets/WebSocket.idl:67 >>> + [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException); >> >> Do we really need custom binding code? >> Probably we can avoid [Custom] by providing close(ExceptionCode&), close(unsigned short, ExceptionCode&), and close(unsigned short, const String&, ExceptionCode&) in WebSocket. > > Main purpose to use [Custom] is to realize [Clamp] attribute behavior exactly. > Autogenerated code will handle casting from Number to unsigned short as modulo. So the right direction is to implement [Clamp] for our binding code generator.

Kent-san, Thank you for review. I totally agree with you. But, could you permit me to split that work to another change? I'd like to try my current implementation here, then apply it to the generator. To my understanding, this is the first and only use case of [Clamp] in WebKit for now.

(In reply to comment #21) > But, could you permit me to split that work to another change? > I'd like to try my current implementation here, then apply it to the generator. It's reasonable. Please file a bug for the generator change now, and add FIXME comments to JSWebSocketCustom.cpp, V8WebSocketCustom.cpp, and WebSocket.idl.

Created attachment 106092 [details] Patch

Comment on attachment 106092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106092&action=review > Source/WebCore/websockets/WebSocket.idl:67 > - void close(); > + [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException); Add a FIXME comment about Clamp please.

Created attachment 106093 [details] Patch

Comment on attachment 106093 [details] Patch ok

Comment on attachment 106093 [details] Patch Clearing flags on attachment: 106093 Committed r94387: <http://trac.webkit.org/changeset/94387>

All reviewed patches have been landed. Closing bug.

(In reply to comment #22) > (In reply to comment #21) > > But, could you permit me to split that work to another change? > > I'd like to try my current implementation here, then apply it to the generator. > > It's reasonable. > Please file a bug for the generator change now, and add FIXME comments to JSWebSocketCustom.cpp, V8WebSocketCustom.cpp, and WebSocket.idl. Filed Bug 77605.