WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
35573
WebSocket add new event: CloseEvent
https://bugs.webkit.org/show_bug.cgi?id=35573
Summary
WebSocket add new event: CloseEvent
Fumitoshi Ukai
Reported
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
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Fumitoshi Ukai
Comment 1
2010-03-02 23:15:25 PST
Created
attachment 49882
[details]
Patch
Alexey Proskuryakov
Comment 2
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.
Fumitoshi Ukai
Comment 3
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
Kent Hansen
Comment 4
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.)
Nikolas Zimmermann
Comment 5
2010-07-30 22:27:38 PDT
Comment on
attachment 49882
[details]
Patch r- based on Kent Hansens comments.
Fumitoshi Ukai
Comment 6
2010-08-01 23:51:02 PDT
Created
attachment 63181
[details]
Patch
Yuta Kitamura
Comment 7
2011-04-04 00:04:22 PDT
Created
attachment 88034
[details]
Merge to trunk
Hajime Morrita
Comment 8
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.
Yuta Kitamura
Comment 9
2011-04-15 00:11:31 PDT
Created
attachment 89750
[details]
Add #ifdef guards
Yuta Kitamura
Comment 10
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.
Yuta Kitamura
Comment 11
2011-04-15 00:49:36 PDT
Created
attachment 89751
[details]
Fix EFL port
Collabora GTK+ EWS bot
Comment 12
2011-04-15 06:04:57 PDT
Attachment 89751
[details]
did not build on gtk: Build output:
http://queues.webkit.org/results/8452129
Yuta Kitamura
Comment 13
2011-04-15 06:56:15 PDT
Created
attachment 89780
[details]
Fix GTK port
Yuta Kitamura
Comment 14
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.)
Alexey Proskuryakov
Comment 15
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.
Yuta Kitamura
Comment 16
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?
Hajime Morrita
Comment 17
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.
Yuta Kitamura
Comment 18
2011-05-11 01:56:14 PDT
Created
attachment 93090
[details]
Put script test to HTML
Yuta Kitamura
Comment 19
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.
Ryosuke Niwa
Comment 20
2011-05-11 02:00:50 PDT
This seems like a feature you should be announcing on webkit-dev.
Early Warning System Bot
Comment 21
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
Yuta Kitamura
Comment 22
2011-05-11 02:19:22 PDT
Created
attachment 93092
[details]
Fix Qt build
WebKit Review Bot
Comment 23
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
WebKit Review Bot
Comment 24
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
WebKit Review Bot
Comment 25
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
Yuta Kitamura
Comment 26
2011-05-11 05:03:22 PDT
Created
attachment 93107
[details]
Update Chromium test results
Yuta Kitamura
Comment 27
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.
Kent Tamura
Comment 28
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().
Yuta Kitamura
Comment 29
2011-05-11 22:10:20 PDT
Created
attachment 93244
[details]
Ready for commit
WebKit Commit Bot
Comment 30
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
>
WebKit Commit Bot
Comment 31
2011-05-12 00:06:43 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 32
2011-05-12 00:38:50 PDT
http://trac.webkit.org/changeset/86315
might have broken Qt Linux Release minimal
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug