Bug 35573 – WebSocket add new event: CloseEvent

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

WebSocket spec adds new event: CloseEvent
http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003976.html

[Fumitoshi Ukai]

Created attachment 49882 [details]
Patch

[Alexey Proskuryakov]

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.

[Fumitoshi Ukai]

(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

[Kent Hansen]

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

[Nikolas Zimmermann]

Comment on attachment 49882 [details]
Patch

r- based on Kent Hansens comments.

[Fumitoshi Ukai]

Created attachment 63181 [details]
Patch

[Yuta Kitamura]

Created attachment 88034 [details]
Merge to trunk

[Hajime Morrita]

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.

[Yuta Kitamura]

Created attachment 89750 [details]
Add #ifdef guards

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 89751 [details]
Fix EFL port

[Collabora GTK+ EWS bot]

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

[Yuta Kitamura]

Created attachment 89780 [details]
Fix GTK port

[Yuta Kitamura]

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

[Alexey Proskuryakov]

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.

[Yuta Kitamura]

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?

[Hajime Morrita]

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

[Yuta Kitamura]

Created attachment 93090 [details]
Put script test to HTML

[Yuta Kitamura]

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

[Ryosuke Niwa]

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

[Early Warning System Bot]

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

[Yuta Kitamura]

Created attachment 93092 [details]
Fix Qt build

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Yuta Kitamura]

Created attachment 93107 [details]
Update Chromium test results

[Yuta Kitamura]

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

[Kent Tamura]

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

[Yuta Kitamura]

Created attachment 93244 [details]
Ready for commit

[WebKit Commit Bot]

Comment on attachment 93244 [details]
Ready for commit

Clearing flags on attachment: 93244

Committed r86315: <http://trac.webkit.org/changeset/86315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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