Created attachment 49882 [details] Patch

Sorry for not reviewing this patch for so long. I understand that this patch is just adding a new event type, but I'll need to refresh my memory on TCP/IP connection closing sequence to meaningfully review the parts of spec it's needed for.

(In reply to comment #2) > Sorry for not reviewing this patch for so long. > > I understand that this patch is just adding a new event type, but I'll need to > refresh my memory on TCP/IP connection closing sequence to meaningfully review > the parts of spec it's needed for. Yes, this adds CloseEvent only. I'll address closing handshake written in 1.4 closing handshake of http://www.whatwg.org/specs/web-socket-protocol/ in https://bugs.webkit.org/show_bug.cgi?id=35721

(In reply to comment #1) > Created an attachment (id=49882) [details] > Patch Unfortunately this patch no longer cleanly applies against trunk. Still, the test case passes even without the rest of the patch; WebSocket already generates a generic ("simple") event with name "close". In order to test the requirement "The user agent must create an event that uses the CloseEvent interface", the test case should check that the event has a read-only property called wasClean, and that the event inherits from CloseEvent. (How to do the latter, I'm not sure, since the CloseEvent constructor itself is not exposed; maybe by converting Object.getPrototypeOf(e) to a string and checking if it equals "[object CloseEventPrototype]"? And/or making sure that the prototype is _not_ equal to Event.prototype.)

Comment on attachment 49882 [details] Patch r- based on Kent Hansens comments.

Created attachment 63181 [details] Patch

Created attachment 88034 [details] Merge to trunk

Comment on attachment 88034 [details] Merge to trunk View in context: https://bugs.webkit.org/attachment.cgi?id=88034&action=review Overall, this patch looks good and has little impact to existing behavior because we already fire the (generic) event for same timing. This is just a refinement of event type. > Source/WebCore/page/DOMWindow.idl:534 > + attribute CloseEventConstructor CloseEvent; Is it OK not to guard with ENABLE(WEB_SOCKETS) here? > Source/WebCore/websockets/CloseEvent.h:31 > +#ifndef CloseEvent_h Is it OK not to guard with ENABLE(WEB_SOCKETS) here? > Source/WebCore/websockets/CloseEvent.h:39 > +class CloseEvent : public Event { Don't you need "reason" or "code" ? http://dev.w3.org/html5/websockets/ (Editor's Draft 12) Is this different version from what you are implementing? (I don't say you are wrong. I just don't have any idea around here.) > Source/WebCore/websockets/WebSocket.cpp:288 > + dispatchEvent(event); You can define another version of create() with accepts other arguments, and omit the temporal variables and initCloseEvent() call here.

Created attachment 89750 [details] Add #ifdef guards

(In reply to comment #8) > > Source/WebCore/page/DOMWindow.idl:534 > > + attribute CloseEventConstructor CloseEvent; > > Is it OK not to guard with ENABLE(WEB_SOCKETS) here? Fixed. > > > Source/WebCore/websockets/CloseEvent.h:31 > > +#ifndef CloseEvent_h > > Is it OK not to guard with ENABLE(WEB_SOCKETS) here? Fixed. > > > Source/WebCore/websockets/CloseEvent.h:39 > > +class CloseEvent : public Event { > > Don't you need "reason" or "code" ? http://dev.w3.org/html5/websockets/ (Editor's Draft 12) > Is this different version from what you are implementing? > (I don't say you are wrong. I just don't have any idea around here.) These two attributes were added recently in response to the new feature added in WebSocket protocol draft (error code and reason in close frame). We don't support this new protocol yet, so I don't think I need to add them now. I will add these two after I update the protocol implementation. > > > Source/WebCore/websockets/WebSocket.cpp:288 > > + dispatchEvent(event); > > You can define another version of create() with accepts other arguments, and omit the temporal variables and initCloseEvent() call here. I think this "calling init***Event() after constructor" pattern is common among many other Events. So I guess we don't need to meld these parameters into constructor.

Created attachment 89751 [details] Fix EFL port

Attachment 89751 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8452129

Created attachment 89780 [details] Fix GTK port

Could anybody review this? This patch introduces a new event defined in WebSocket API draft. I think definition of CloseEvent is already mature. http://dev.w3.org/html5/websockets/#event-definitions (As mentioned in comment #10, our WebSocket implementation does not provide error code and reason string (yet), so "code" and "reason" attributes are not added in this patch.)

Comment on attachment 89780 [details] Fix GTK port View in context: https://bugs.webkit.org/attachment.cgi?id=89780&action=review > LayoutTests/http/tests/websocket/tests/close-event.html:11 > +<script src="script-tests/close-event.js"></script> Please don't split tests in two parts.

Comment on attachment 89780 [details] Fix GTK port View in context: https://bugs.webkit.org/attachment.cgi?id=89780&action=review >> LayoutTests/http/tests/websocket/tests/close-event.html:11 >> +<script src="script-tests/close-event.js"></script> > > Please don't split tests in two parts. Do you mean that script-tests (generated by make-script-test-wrappers) are deprecated? Really?

> Do you mean that script-tests (generated by make-script-test-wrappers) are deprecated? Really? Yes. Personally it was a bit sad. But having test code in single file actually helps, especially seeing it in flakiness dashboard. You can use js-test-pre.js family as before.

Created attachment 93090 [details] Put script test to HTML

(In reply to comment #17) > > Do you mean that script-tests (generated by make-script-test-wrappers) are deprecated? Really? > Yes. > Personally it was a bit sad. But having test code in single file actually helps, > especially seeing it in flakiness dashboard. > You can use js-test-pre.js family as before. Sure, fixed.

This seems like a feature you should be announcing on webkit-dev.

Comment on attachment 93090 [details] Put script test to HTML Attachment 93090 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8687267

Created attachment 93092 [details] Fix Qt build

Comment on attachment 93092 [details] Fix Qt build Attachment 93092 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8687278 New failing tests: fast/dom/prototype-inheritance.html

Created attachment 93099 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 93090 [details] Put script test to HTML Attachment 93090 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8688266

Created attachment 93107 [details] Update Chromium test results

(In reply to comment #20) > This seems like a feature you should be announcing on webkit-dev. I have sent a mail to webkit-dev.

Comment on attachment 93107 [details] Update Chromium test results View in context: https://bugs.webkit.org/attachment.cgi?id=93107&action=review > LayoutTests/http/tests/websocket/tests/close-event.html:22 > +if (window.layoutTestController) > + layoutTestController.waitUntilDone(); > + > +function endTest() > +{ > + isSuccessfullyParsed(); > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > +} We can simplify this by window.jsTestIsAsync = true; and replacing endTest() with finishJSTest().

Created attachment 93244 [details] Ready for commit

Comment on attachment 93244 [details] Ready for commit Clearing flags on attachment: 93244 Committed r86315: <http://trac.webkit.org/changeset/86315>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/86315 might have broken Qt Linux Release minimal