Patrick McManus [:mcmanus] Assignee
	Description • 13 years ago

our closeEvent is missing the code and reason attributes, additionally the close method (for sending closes) is missing the same optional stuff.

The protocol defines a number of standard reason codes.

	Patrick McManus [:mcmanus] Assignee
	Comment 1 • 13 years ago

includes tests.. :sicking r? for dom, I'll add a r? biesi for the network side

	Patrick McManus [:mcmanus] Assignee
	Comment 2 • 13 years ago

try server showed me that v1 didn't build on windows due to an extra const. That's the only change.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 3 • 13 years ago

Comment on attachment 549008 [details] [diff] [review]
patch v2

Review of attachment 549008 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsWebSocket.cpp
@@ +835,5 @@
>  
>    rv = conn->Init(this);
>    mConnection = conn;
>    if (NS_FAILED(rv)) {
> +    Close(0, NS_LITERAL_STRING(""), 0);

EmptyString()

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +87,5 @@
>  
>    rv = mChannel->SetNotificationCallbacks(this);
>    if (NS_FAILED(rv)) {
> +    mChannel->Close(nsIWebSocketChannel::CLOSE_ABNORMAL,
> +                    NS_LITERAL_CSTRING(""));

EmptyString() here and on the other lines

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 13 years ago

Comment on attachment 549008 [details] [diff] [review]
patch v2

Review of attachment 549008 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: content/base/src/nsWebSocket.cpp
@@ +1306,5 @@
> +    if (NS_IS_HIGH_SURROGATE(aData[i])) {
> +      if (i + 1 == length || !NS_IS_LOW_SURROGATE(aData[i + 1])) {
> +        return PR_TRUE;
> +      }
> +      ++i;

Nit: Slightly faster as:

++i;
if (i == length || !NS_IS_LOW_SURROGATE(aData[i])) {
  ...

@@ +1352,5 @@
> +  }
> +
> +  if (argc >= 2) {
> +    if (ContainsUnpairedSurrogates(reason))
> +      return NS_ERROR_DOM_SYNTAX_ERR;

Hmm.. If we bail here end up setting mClientReasonCode and possibly reusing it on a later call to close() which seems wrong.

@@ +1358,5 @@
> +    CopyUTF16toUTF8(reason, mClientReason);
> +
> +    // The API requires the UTF-8 string to be 123 or less bytes
> +    if (mClientReason.Length() > 123)
> +      return NS_ERROR_DOM_SYNTAX_ERR;

Same here but with both mClientReason and mClientReasonCode set. We should probably set any of them if all checks pass.

Also please add a test that catches this.

	Patrick McManus [:mcmanus] Assignee
	Comment 5 • 13 years ago

review comments incorporated .. seeking sr due to API change

	Patrick McManus [:mcmanus] Assignee
	Comment 6 • 13 years ago

Comment on attachment 550393 [details] [diff] [review]
patch 3

that new test I added at jonas's request isn't quite right upon further reflection.. I'll update and re sr?

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 13 years ago

Comment on attachment 550397 [details] [diff] [review]
v4

sr=me

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 9 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/e054dec99f3e

	Eric Shepherd [:sheppy]
	Comment 10 • 13 years ago

Documentation updated:

https://developer.mozilla.org/en/WebSockets/WebSockets_reference/CloseEvent
https://developer.mozilla.org/en/WebSockets/WebSockets_reference/WebSocket

New pages added; these were not previously documented.

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWebSocketChannelhttps://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWebSocketListener

Firefox 8 for developers has been updated as well.