Closed
Bug 674716
Opened 12 years ago
Closed 12 years ago
Update WebSocket API to latest draft - close codes and reasons
Categories
(Core :: Networking: WebSockets, defect)
Core
Networking: WebSockets
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [inbound])
Attachments
(1 file, 3 obsolete files)
45.53 KB,
patch
|
mcmanus
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
includes tests.. :sicking r? for dom, I'll add a r? biesi for the network side
Assignee | ||
Updated•12 years ago
|
Attachment #548926 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 2•12 years ago
|
||
try server showed me that v1 didn't build on windows due to an extra const. That's the only change.
Attachment #548926 -
Attachment is obsolete: true
Attachment #548926 -
Flags: review?(jonas)
Attachment #548926 -
Flags: review?(cbiesinger)
Attachment #549008 -
Flags: review?(jonas)
Assignee | ||
Updated•12 years ago
|
Attachment #549008 -
Flags: review?(cbiesinger)
Comment 3•12 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
Attachment #549008 -
Flags: review?(cbiesinger) → review+
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.
Attachment #549008 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 5•12 years ago
|
||
review comments incorporated .. seeking sr due to API change
Attachment #549008 -
Attachment is obsolete: true
Attachment #550393 -
Flags: superreview?
Attachment #550393 -
Flags: review+
Assignee | ||
Comment 6•12 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?
Attachment #550393 -
Flags: superreview?
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #550393 -
Attachment is obsolete: true
Attachment #550397 -
Flags: superreview?(bzbarsky)
Attachment #550397 -
Flags: review+
![]() |
||
Comment 8•12 years ago
|
||
Comment on attachment 550397 [details] [diff] [review] v4 sr=me
Attachment #550397 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Comment 9•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e054dec99f3e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 10•12 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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•