Initial Implementation of WebSocket Compression

Initial Implementation of WebSocket Compression

BUG: https://code.google.com/p/dart/issues/detail?id=14441

[Søren Gjesse, 2015-05-26 09:11:32]

sgjesse@google.com changed reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2015-05-26 09:11:33]

Hi Kenneth,

Thanks for working on this!

I have added some initial comments. Overall it looks pretty good.

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
File sdk/lib/io/websocket_impl.dart (right):

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:288: if (_fin) {
Move getting bytes and optionally decompression up here.

You also need to check the RSV1 bit to check whether to decompress.

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:424: Iterable<List<String>> extensions =
extensionHeader.split(",").map((it) => it.split("; "));
I think it will be helpful to create a class to represent the
Sec-WebSocket-Extensions header, e.g. like the ContentType for the HTTP
Content-Type
(https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:io.Conte...).

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:424: Iterable<List<String>> extensions =
extensionHeader.split(",").map((it) => it.split("; "));
Long line - more below.

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:436: socket, protocol, true).._deflateHelper =
deflateHelper);
Please pass the arguments used for handling compression to the constructor, and
let the constructor create the deflater.

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:493: class _WebSocketPerMessageDeflateHelper {
I would just call this _WebSocketPerMessageDeflater.

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:571: }
Move the compression down here to avoid the duplication (checking if data !=
null).

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:874: ..set("Sec-WebSocket-Extensions",
"permessage-deflate");
There should be some way of controlling aspects of the per message compression.

E.g. for the context takeover and window bits. Not sure how to best do this,
maybe a CompressionOptions object that can be passed with some pre-defined
instances, e.g.

class CompressionOptions {
  CompressionOptions DEFAULT = const CompressionOptions(...);
  CompressionOptions SAVE_CLIENT_MEMORY = const CompressionOptions(...);
  ...

  CompressionOptions(...);
}

https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
sdk/lib/io/websocket_impl.dart:925:
response.headers.add("Sec-WebSocket-Extensions", "permessage-deflate");
Even for the simplest implementation you need to support
'client_no_context_takeover'. The spec says:

A client MUST support the "client_no_context_takeover" extension parameter in an
extension negotiation response

in section 8.1.1.2.

[sethladd, 2015-05-26 15:52:09]

Thanks for the review, Soren!

[kaendfinger, 2015-05-26 16:23:04]

On 2015/05/26 09:11:33, Søren Gjesse wrote:
> Hi Kenneth,
> 
> Thanks for working on this!
> 
> I have added some initial comments. Overall it looks pretty good.
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> File sdk/lib/io/websocket_impl.dart (right):
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:288: if (_fin) {
> Move getting bytes and optionally decompression up here.
> 
> You also need to check the RSV1 bit to check whether to decompress.
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:424: Iterable<List<String>> extensions =
> extensionHeader.split(",").map((it) => it.split("; "));
> I think it will be helpful to create a class to represent the
> Sec-WebSocket-Extensions header, e.g. like the ContentType for the HTTP
> Content-Type
>
(https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:io.Conte...).
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:424: Iterable<List<String>> extensions =
> extensionHeader.split(",").map((it) => it.split("; "));
> Long line - more below.
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:436: socket, protocol, true).._deflateHelper =
> deflateHelper);
> Please pass the arguments used for handling compression to the constructor,
and
> let the constructor create the deflater.
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:493: class _WebSocketPerMessageDeflateHelper {
> I would just call this _WebSocketPerMessageDeflater.
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:571: }
> Move the compression down here to avoid the duplication (checking if data !=
> null).
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:874: ..set("Sec-WebSocket-Extensions",
> "permessage-deflate");
> There should be some way of controlling aspects of the per message
compression.
> 
> E.g. for the context takeover and window bits. Not sure how to best do this,
> maybe a CompressionOptions object that can be passed with some pre-defined
> instances, e.g.
> 
> class CompressionOptions {
>   CompressionOptions DEFAULT = const CompressionOptions(...);
>   CompressionOptions SAVE_CLIENT_MEMORY = const CompressionOptions(...);
>   ...
> 
>   CompressionOptions(...);
> }
> 
>
https://codereview.chromium.org/1158593002/diff/20001/sdk/lib/io/websocket_im...
> sdk/lib/io/websocket_impl.dart:925:
> response.headers.add("Sec-WebSocket-Extensions", "permessage-deflate");
> Even for the simplest implementation you need to support
> 'client_no_context_takeover'. The spec says:
> 
> A client MUST support the "client_no_context_takeover" extension parameter in
an
> extension negotiation response
> 
> in section 8.1.1.2.

Thanks for reviewing the initial go around! I'll start working on the rest of
this soon!