Closed Bug 718557 Opened 11 years ago Closed 10 years ago

Clean up DOM Websocket close logic.


(Core :: Networking: WebSockets, defect)

Not set





(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)



(1 file, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This patch does a bunch of related things:

1) It cleans up nsWebSocket's CloseConnection and FailConnection so that they
   explictly take close code/reason arguments, rather than rely on member
   variables.  It's always clear at call time what error code should be passed,
   so there's no need to use member variables.  Makes code cleaner & safer IMO.

2) Adds a new CLOSE_INTERNAL_ERROR code that we return whenever we close the WS
   connection due to some internal problem (malloc fails, can't get
   charConverter, etc).  The WS spec suprisingly doesn't have any equivalent of
   the HTTP 500 (server internal error) code.  On consultation with Patrick
   McManus we're going to go with 1001 ("endpoint is going away").  Previously
   we were responding with whatever mClientReasonCode was set to (by default it
   was 0 and we sent no close code at all).

3) Changes OnServerClose so that when we receive a server close, we reply with a
   close message that has the same code/reason as the server provide, as the
   spec recommends (previously we were sending whatever mClientReasonCode was
   set to--usually 0, so no close code at all).  

4) Gets rid of m{Client|Server}Reason{Code}, which are no longer needed.
   Centralize naming of mCloseEventXXX variables, and give them names that match
   the spec for code clarity.
Attachment #589100 - Flags: review?(bugs)
Comment on attachment 589100 [details] [diff] [review]

>+nsWebSocket::FailConnection(PRUint16 reasonCode) {
>+    return FailConnection(reasonCode, NS_LITERAL_CSTRING(""));
>+inline nsresult
>+nsWebSocket::CloseConnection(PRUint16 reasonCode) {
>+    return CloseConnection(reasonCode, NS_LITERAL_CSTRING(""));

EmptyCString() please. Could you actually have
the methods like
CloseConnection(PRUint16 reasonCode, const nsAString& aReason = EmptyCString())
Not sure if that compiles.
Also, aReasonCode. Parameters start with 'a'

Sorry for the delay.
Attachment #589100 - Flags: review?(bugs) → review-
Attached patch v2 (obsolete) — Splinter Review
all fixes made.
Attachment #589100 - Attachment is obsolete: true
Attachment #592259 - Flags: review?(bugs)
Attached patch v2 (for realz)Splinter Review
forgot to refresh mq patch
Attachment #592259 - Attachment is obsolete: true
Attachment #592259 - Flags: review?(bugs)
Attachment #592261 - Flags: review?(bugs)
Attachment #592261 - Attachment description: v2 (for realiz) → v2 (for realz)
Attachment #592261 - Flags: review?(bugs) → review+
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.