Closed Bug 674905 Opened 13 years ago Closed 13 years ago

Update WebSocket API to latest draft - extension handling

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #666349 +++ The current implementation of the WebSocket interface does not match the current specification in several ways: as of 7/28/2011 http://dev.w3.org/html5/websockets/ defines an "extensions" attribute that lists the negotiated extensions. It also prohibits the client implementing that API from sending any requested extensions. This disables compressions, which is imo the wrong thing to do. nonetheless, this patch makes us toe that line and implement the attribute.
Blocks: 666349
No longer depends on: 666349, 674527, 674716, 674890
Summary: Update WebSocket API to latest draft → Update WebSocket API to latest draft - extension handling
Attached patch patch v1 (obsolete) (deleted) — Splinter Review
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #549189 - Flags: review?(jonas)
Comment on attachment 549189 [details] [diff] [review] patch v1 Christan, can you take the netwerk bits?
Attachment #549189 - Flags: review?(cbiesinger)
Attachment #549189 - Flags: review?(cbiesinger) → review+
> This disables compressions, which is imo the wrong thing to do. Apparently the IETF folks think there's a fundamental problem with the compression stuff in the protocol right now, hence this API choice. When the IETF has a compression method they like, the protocol will use it...
(In reply to comment #4) > > This disables compressions, which is imo the wrong thing to do. > > Apparently the IETF folks think there's a fundamental problem with the > compression stuff in the protocol right now, hence this API choice. When > the IETF has a compression method they like, the protocol will use it... I am one of those ietf folk. The pluggable compression algorithm can be improved - for sure. There are certain interactions with masking that hurt its efficacy (basically really short messages in the upstream direction). But there are plenty of other scenarios where it still helps and it is never a disaster. The right thing to do in my opinion is to require that we request compression, generically speaking instead of any algorithm. And then update as the new definitions become available. (the protocol would allow us to request 2 and the server would only return the best available).
That seems fine; can we get that into the specs?
I'm fine with us supporting the current compression algorithm that we support now, and once there is a better (good) compression algorithm implement that one and drop the one we support now. The nice thing about that strategy is that websites can rely on some form of compression being available and don't have to compress in JS. However once that new algorithm is available, I think the API spec should be updated to require that browsers support that and only that. It'd be nice if the spec was updated to be ok with this strategy, but I also don't think the current text in the spec should prevent us from doing what we think is right.
(In reply to comment #7) > I'm fine with us supporting the current compression algorithm that we > support now, and once there is a better (good) compression algorithm > implement that one and drop the one we support now. > > The nice thing about that strategy is that websites can rely on some form of > compression being available and don't have to compress in JS. > > However once that new algorithm is available, I think the API spec should be > updated to require that browsers support that and only that. > I think that's a good approach. I'll update the patch. (the patch is still necessary to make the negotiated extension property readadble in js as defined by the api.)
Attached patch patch 2 (deleted) — Splinter Review
Attachment #549189 - Attachment is obsolete: true
Attachment #549189 - Flags: review?(jonas)
Attachment #550475 - Flags: review?(jonas)
Comment on attachment 550475 [details] [diff] [review] patch 2 API change makes a new attribute available in the IDL
Attachment #550475 - Flags: superreview?(bzbarsky)
Comment on attachment 550475 [details] [diff] [review] patch 2 r=me
Attachment #550475 - Flags: superreview?(bzbarsky) → superreview+
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Documentation updated: https://developer.mozilla.org/en/WebSockets/WebSockets_reference/WebSocket#Gecko_notes And referenced on Firefox 8 for developers.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: