Bug 35573 - WebSocket add new event: CloseEvent
Summary: WebSocket add new event: CloseEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Fumitoshi Ukai
URL:
Keywords:
Depends on:
Blocks: 35721 50099
  Show dependency treegraph
 
Reported: 2010-03-02 03:18 PST by Fumitoshi Ukai
Modified: 2011-05-12 00:38 PDT (History)
15 users (show)

See Also:


Attachments
Patch (24.63 KB, patch)
2010-03-02 23:15 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (24.22 KB, patch)
2010-08-01 23:51 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Merge to trunk (31.89 KB, patch)
2011-04-04 00:04 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Add #ifdef guards (31.94 KB, patch)
2011-04-15 00:11 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Fix EFL port (32.42 KB, patch)
2011-04-15 00:49 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Fix GTK port (32.28 KB, patch)
2011-04-15 06:56 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Put script test to HTML (31.76 KB, patch)
2011-05-11 01:56 PDT, Yuta Kitamura
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix Qt build (31.75 KB, patch)
2011-05-11 02:19 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.15 MB, application/zip)
2011-05-11 02:58 PDT, WebKit Review Bot
no flags Details
Update Chromium test results (32.64 KB, patch)
2011-05-11 05:03 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Ready for commit (32.46 KB, patch)
2011-05-11 22:10 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-03-02 03:18:56 PST
WebSocket spec adds new event: CloseEvent
http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003976.html
Comment 1 Fumitoshi Ukai 2010-03-02 23:15:25 PST
Created attachment 49882 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-03-16 10:09:19 PDT
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.
Comment 3 Fumitoshi Ukai 2010-03-16 18:13:15 PDT
(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
Comment 4 Kent Hansen 2010-07-26 04:15:45 PDT
(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 5 Nikolas Zimmermann 2010-07-30 22:27:38 PDT
Comment on attachment 49882 [details]
Patch

r- based on Kent Hansens comments.
Comment 6 Fumitoshi Ukai 2010-08-01 23:51:02 PDT
Created attachment 63181 [details]
Patch
Comment 7 Yuta Kitamura 2011-04-04 00:04:22 PDT
Created attachment 88034 [details]
Merge to trunk
Comment 8 Hajime Morrita 2011-04-12 13:55:15 PDT
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.
Comment 9 Yuta Kitamura 2011-04-15 00:11:31 PDT
Created attachment 89750 [details]
Add #ifdef guards
Comment 10 Yuta Kitamura 2011-04-15 00:22:11 PDT
(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.
Comment 11 Yuta Kitamura 2011-04-15 00:49:36 PDT
Created attachment 89751 [details]
Fix EFL port
Comment 12 Collabora GTK+ EWS bot 2011-04-15 06:04:57 PDT
Attachment 89751 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8452129
Comment 13 Yuta Kitamura 2011-04-15 06:56:15 PDT
Created attachment 89780 [details]
Fix GTK port
Comment 14 Yuta Kitamura 2011-04-19 19:57:07 PDT
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 15 Alexey Proskuryakov 2011-04-26 15:33:07 PDT
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 16 Yuta Kitamura 2011-04-26 16:01:12 PDT
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?
Comment 17 Hajime Morrita 2011-05-11 01:22:56 PDT
> 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.
Comment 18 Yuta Kitamura 2011-05-11 01:56:14 PDT
Created attachment 93090 [details]
Put script test to HTML
Comment 19 Yuta Kitamura 2011-05-11 01:57:49 PDT
(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.
Comment 20 Ryosuke Niwa 2011-05-11 02:00:50 PDT
This seems like a feature you should be announcing on webkit-dev.
Comment 21 Early Warning System Bot 2011-05-11 02:11:15 PDT
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
Comment 22 Yuta Kitamura 2011-05-11 02:19:22 PDT
Created attachment 93092 [details]
Fix Qt build
Comment 23 WebKit Review Bot 2011-05-11 02:58:48 PDT
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
Comment 24 WebKit Review Bot 2011-05-11 02:58:54 PDT
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 25 WebKit Review Bot 2011-05-11 03:31:06 PDT
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
Comment 26 Yuta Kitamura 2011-05-11 05:03:22 PDT
Created attachment 93107 [details]
Update Chromium test results
Comment 27 Yuta Kitamura 2011-05-11 06:36:31 PDT
(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 28 Kent Tamura 2011-05-11 18:33:43 PDT
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().
Comment 29 Yuta Kitamura 2011-05-11 22:10:20 PDT
Created attachment 93244 [details]
Ready for commit
Comment 30 WebKit Commit Bot 2011-05-12 00:06:32 PDT
Comment on attachment 93244 [details]
Ready for commit

Clearing flags on attachment: 93244

Committed r86315: <http://trac.webkit.org/changeset/86315>
Comment 31 WebKit Commit Bot 2011-05-12 00:06:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 WebKit Review Bot 2011-05-12 00:38:50 PDT
http://trac.webkit.org/changeset/86315 might have broken Qt Linux Release minimal