Last Comment Bug 674905 - Update WebSocket API to latest draft - extension handling
: Update WebSocket API to latest draft - extension handling
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
: 675934 (view as bug list)
Depends on:
Blocks: 666349
  Show dependency treegraph
 
Reported: 2011-07-28 07:39 PDT by Patrick McManus [:mcmanus]
Modified: 2014-06-23 18:37 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (15.43 KB, patch)
2011-07-28 12:01 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
patch 2 (14.19 KB, patch)
2011-08-03 13:23 PDT, Patrick McManus [:mcmanus]
jonas: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2011-07-28 07:39:40 PDT
+++ 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] 2011-07-28 12:01:48 PDT
Created attachment 549189 [details] [diff] [review]
patch v1
Comment 2 Patrick McManus [:mcmanus] 2011-07-28 12:02:19 PDT
Comment on attachment 549189 [details] [diff] [review]
patch v1

Christan, can you take the netwerk bits?
Comment 3 Patrick McManus [:mcmanus] 2011-08-02 07:59:30 PDT
*** Bug 675934 has been marked as a duplicate of this bug. ***
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-08-02 08:02:11 PDT
> 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] 2011-08-02 08:10:42 PDT
(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 [:bz] (still a bit busy) 2011-08-02 08:17:03 PDT
That seems fine; can we get that into the specs?
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-02 20:10:13 PDT
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] 2011-08-03 13:22:46 PDT
(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] 2011-08-03 13:23:16 PDT
Created attachment 550475 [details] [diff] [review]
patch 2
Comment 10 Patrick McManus [:mcmanus] 2011-08-03 17:19:21 PDT
Comment on attachment 550475 [details] [diff] [review]
patch 2

API change makes a new attribute available in the IDL
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-08-03 20:12:47 PDT
Comment on attachment 550475 [details] [diff] [review]
patch 2

r=me
Comment 12 Marco Bonardo [::mak] 2011-08-04 03:20:18 PDT
http://hg.mozilla.org/mozilla-central/rev/5edb33c72ec0
Comment 13 Eric Shepherd [:sheppy] 2011-10-17 13:14:59 PDT
Documentation updated:

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

And referenced on Firefox 8 for developers.

Note You need to log in before you can comment on or make changes to this bug.