Summary: | [WebSocket] WebSocket object should fire a simple event named error when it is required to fail the websocket connection. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Li Yin <li.yin> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, bashi, tkent, toyoshim, webkit.review.bot, yutak | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Li Yin
2012-05-20 01:10:04 PDT
Created attachment 142903 [details]
Patch
Hi Li, Thank you for sending a patch. Actually, this is a bug and should be fixed. A filed bug in chromium side issue tracker is here. http://code.google.com/p/chromium/issues/detail?id=128057 I think original code just missed to call WebSocketChannelClient::didReceiveMessageError() in WebSocketChannel::fail(). Yuta, I think you write this part. Could you check this? Seems like the spec changed at http://html5.org/tools/web-apps-tracker?from=6376&to=6377 (see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=13294) As Takashi said, I think you just need to call didReceiveMessageError() in fail(). You don't have to add a new function to WebSocketChannelClient. You probably need to have a line like "RefPtr<WebSocketChannel> protect;" before calling didReceiveMessageError(), otherwise this function may cause the channel to get removed (because users may do "ws.close()" in onerror handler). Also, you might need to change the definition of didReceiveMessageError() to let it fire the event when the state is not OPEN or CLOSING. This is old code based on the old API spec, and I guess this restriction is leftover from the old age. Comment on attachment 142903 [details]
Patch
r- because adding a new callback fireErrorEvent() is clearly unacceptable. fireErrorEvent() doesn't work properly in Web Workers, as the definition for workers isn't provided.
(In reply to comment #3) > Seems like the spec changed at http://html5.org/tools/web-apps-tracker?from=6376&to=6377 > (see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=13294) > > As Takashi said, I think you just need to call didReceiveMessageError() in fail(). You don't have to add a new function to WebSocketChannelClient. > > You probably need to have a line like "RefPtr<WebSocketChannel> protect;" before calling didReceiveMessageError(), otherwise this function may cause the channel to get removed (because users may do "ws.close()" in onerror handler). > > Also, you might need to change the definition of didReceiveMessageError() to let it fire the event when the state is not OPEN or CLOSING. This is old code based on the old API spec, and I guess this restriction is leftover from the old age. Thanks for your review and good solution, I have updated the patch, please review it again. Created attachment 143210 [details]
Patch
Comment on attachment 143210 [details]
Patch
Looks fine. Please wait for comments from a reviewer.
Comment on attachment 143210 [details] Patch Clearing flags on attachment: 143210 Committed r117944: <http://trac.webkit.org/changeset/117944> All reviewed patches have been landed. Closing bug. |