WebSocket: Return type of send() should be void if hybi-10 protocol is chosen

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

Return type of send() was "bool" in the old WebSocket API draft, but it has become "void" in the latest draft:

http://dev.w3.org/html5/websockets/#the-websocket-interface
  interface WebSocket {
    ...
    void send(in DOMString data);
    void send(in ArrayBuffer data);
    void send(in Blob data);
    ...
  };

Because a Blob must be read asynchronously, sending a Blob is also asynchronous operation. Hence, it is not always possible to know whether the actual send operation was successful or not. This is why the return type has been changed.

[Yuta Kitamura]

Created attachment 103231 [details]
Patch

[Alexey Proskuryakov]

I understand the consistency reason, but changing the API just for blobs (which other browsers may never implement) seems quite unfortunate.

What is the upgrade path for client side code?

[Ian 'Hixie' Hickson]

It wasn't changed just for blobs. In fact it changed over a year ago.

[Alexey Proskuryakov]

What is the upgrade path for client side code?

[Yuta Kitamura]

Another option is to always return true. We may intentionally choose this option just to keep compatibility with older scripts.

[Takeshi Yoshino]

Make it always returning true to address this incompatibility may mislead people who are relying on the return value without setting up onclose and onerror to see. So that's not an option we can take, I think.

On this working draft update, Hixie changed the return type of send to void.
http://www.w3.org/TR/2011/WD-websockets-20110419/
This change was made on editor's draft on Aug 6, 2010. A year ago.
http://dev.w3.org/cvsweb/html5/websockets/Overview.html.diff?r1=1.188;r2=1.189;f=h
It seems that this Simon's post is the rationale for the change. I think this makes sense as a text for specification.
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-April/025942.html

Even before introduction of Blob type to WebSocket, it was not clear a certain send call successfully delivered data to the other peer. The return value can be just a convenient method to see if the connection is closed or not without setting up onclose. The situation is actually not changed. It still make sense to return true/false based on readyState.

But as Mozilla, Opera are disabling WebSocket now and Microsoft doesn't have it yet, they won't hesitate to change the API to the latest one when they reenable WebSocket, I think. Let's go the way we can make the behavior consistent among browsers.

Without the return value, they're still notified of the failure by close event. When we roll out this update, people who were checking the return value just encounter 100% failure in their applications and see that the API is changed to return undefined. All what they need to do is removing return value checking and setup onerror and onclose handler.

[Ian 'Hixie' Hickson]

Older scripts will break anyway because the protocol changed. So I don't think there's a huge compatibility issue here.

[Alexey Proskuryakov]

I don't understand how that's relevant. Protocol change breaks servers, not scripts.

You can make your server support several versions of the protocol to work with different browsers (and that's what servers do in practice). Generally, you just upgrade server code that someone else wrote.

Writing JS code that's backwards compatible will be harder - you can't test return type of send() upfront, for instance. Also, browser vendors were somehow under impression that the API was already stable, so it shipped unprefixed (and was unprefixed in Gecko and Opera before they disabled it).

[Takeshi Yoshino]

(In reply to comment #8)
> Writing JS code that's backwards compatible will be harder - you can't test return type of send() upfront, for instance.

It's fine to assume the return type of send() is void. JS is notified of failure via onclose handler on both HyBi 00 and 10.

[Yuta Kitamura]

Comment on attachment 103231 [details]
Patch

r- because the patch doesn't apply any more.

[Yuta Kitamura]

Gecko 8 has changed the return type to void.

https://developer.mozilla.org/en/WebSockets/WebSockets_reference/WebSocket#send()
> Gecko returns a boolean indicating whether or not the connection is still open; this is corrected in Gecko 8.0 (Firefox 8.0 / Thunderbird 8.0 / SeaMonkey 2.5).

I haven't tried IE10 yet, but I guess it also returns void because the implementation follows the spec which is relatively new.

Given the situation, I think we can (and should) change the return type at this time. Opinions?

[Yuta Kitamura]

I checked out IE 10 prerelease and confirmed that IE's send() returns void.

[Yuta Kitamura]

Created attachment 111442 [details]
Patch v2 (rebase)

[Ojan Vafai]

Comment on attachment 111442 [details]
Patch v2 (rebase)

Patch looks good. Firefox has been shipping this for many versions now and this makes us match the spec. So, seems like a good change.

[Lamarque V. Souza]

Created attachment 198851 [details]
Patch v3 (rebase)

Rebase patch.

[Alexey Proskuryakov]

Comment on attachment 198851 [details]
Patch v3 (rebase)

View in context: https://bugs.webkit.org/attachment.cgi?id=198851&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

This patch has already been reviewed, so you didn't need to ask for review again.

> Source/WebCore/ChangeLog:23
> +        (WebCore::DOMWindowCSS::~DOMWindowCSS):
> +        (DOMWindowCSS):

What?

[Lamarque V. Souza]

Created attachment 198907 [details]
Patch v3 (rebase)

Fix ChangeLog.

[Lamarque V. Souza]

(In reply to comment #16)
> (From update of attachment 198851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198851&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> This patch has already been reviewed, so you didn't need to ask for review again.
> 
> > Source/WebCore/ChangeLog:23
> > +        (WebCore::DOMWindowCSS::~DOMWindowCSS):
> > +        (DOMWindowCSS):
> 
> What?

Sorry, it is leftover. WebKit does not compile with css3-conditional-rules flag enabled anymore [1], I had to fix the problem in DOMWindowCSS to compile WebKit and run the unit tests before submitting this patch. I removed the changes in DOMWindowCSS but forgot to edit the ChangeLog.

[1] https://bugs.webkit.org/show_bug.cgi?id=109926

[Lamarque V. Souza]

(In reply to comment #17)
> Created an attachment (id=198907) [details]
> Patch v3 (rebase)
> 
> Fix ChangeLog.

Hi Alexey, can you cq+ the patch 198907 for me? I am not a committer so I cannot do it myself.

[WebKit Commit Bot]

Comment on attachment 198907 [details]
Patch v3 (rebase)

Clearing flags on attachment: 198907

Committed r148968: <http://trac.webkit.org/changeset/148968>

[Lamarque V. Souza]

Can someone close this bug since the patch already landed? I do not have permissions to do so.