674905 - Update WebSocket API to latest draft - extension handling

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

+++ 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.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch v1

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 549189 [details] [diff] [review]
patch v1

Christan, can you take the netwerk bits?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 5

[Patrick McManus [:mcmanus]]

(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).

Comment 6

[Boris Zbarsky [:bzbarsky]]

That seems fine; can we get that into the specs?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Patrick McManus [:mcmanus]]

(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.)

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: patch 2

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 550475 [details] [diff] [review]
patch 2

API change makes a new attribute available in the IDL

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 550475 [details] [diff] [review]
patch 2

r=me

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5edb33c72ec0

Comment 13

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/WebSockets/WebSockets_reference/WebSocket#Gecko_notes

And referenced on Firefox 8 for developers.