The default bug view has changed. See this FAQ.

Update WebSocket API to latest draft - extension handling

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
+++ 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.
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 549189 [details] [diff] [review]
patch v1
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #549189 - Flags: review?(jonas)
(Assignee)

Comment 2

6 years ago
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+
(Assignee)

Updated

6 years ago
Duplicate of this bug: 675934
> 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...
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
(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.)
(Assignee)

Comment 9

6 years ago
Created attachment 550475 [details] [diff] [review]
patch 2
Attachment #549189 - Attachment is obsolete: true
Attachment #549189 - Flags: review?(jonas)
Attachment #550475 - Flags: review?(jonas)
Attachment #550475 - Flags: review?(jonas) → review+
(Assignee)

Comment 10

6 years ago
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+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/5edb33c72ec0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Documentation updated:

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

And referenced on Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.