Add HttpServer:autoCompress option, to disable auto gzip compression.

Add HttpServer:autoCompress option, to disable auto gzip compression.

BUG=
R=lrn@google.com, sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=40587

[Anders Johnsen, 2014-09-23 07:02:25]

ajohnsen@google.com changed reviewers:
+ lrn@google.com, sgjesse@google.com

[Lasse Reichstein Nielsen, 2014-09-23 07:24:52]

lgtm

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:182: * Get and set if the [HttpServer] should auto-compress
the content, when
Other documentation doesn't use "get and set".
This is a property, so just describe it as a noun (or here, since it's bool, as
a choice), not as its getter/setter's behavior.

"Whether the [HttpServer] will automatically compress the content, if possible."

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:189: * To disable, set to `false`.
Combine the two short paragrahps?
"The default value is `true` (auto-compress enabled). To disable, set
`autoCompress` to `false`."

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:193: bool autoCompress;
If you can enable and disable it, should it still be called "auto", and not just
"compress"?

How about:
  /**
   * Whether the [HttpServer] should compress the content, if possible.
   *
   * The content can only be compressed when the response is using
   * chunked Transfer-Encoding and the incoming request has `gzip` in 
   * as an accepted encoding in the Accept-Encoding header.
   *
   * The default value is `true` (compression enabled). 
   * To disable, set `compress` to false.
   */
  bool compress;

[Søren Gjesse, 2014-09-23 07:56:23]

lgtm

It would be great if we can make a simple way of turning this on on a
per-response basis, but lets keep it like this for now.

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:191: * The default value is `true`.
I know the current default is true, but we should consider making the default
false, so nothing "automatic" happens unless explicitly turned on.

[Anders Johnsen, 2014-09-23 08:35:34]

Made default `false`. PTAL.

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:182: * Get and set if the [HttpServer] should auto-compress
the content, when
On 2014/09/23 07:24:52, Lasse Reichstein Nielsen wrote:
> Other documentation doesn't use "get and set".
> This is a property, so just describe it as a noun (or here, since it's bool,
as
> a choice), not as its getter/setter's behavior.
> 
> "Whether the [HttpServer] will automatically compress the content, if
possible."

Done.

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:189: * To disable, set to `false`.
On 2014/09/23 07:24:52, Lasse Reichstein Nielsen wrote:
> Combine the two short paragrahps?
> "The default value is `true` (auto-compress enabled). To disable, set
> `autoCompress` to `false`."

Done.

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:191: * The default value is `true`.
On 2014/09/23 07:56:23, Søren Gjesse wrote:
> I know the current default is true, but we should consider making the default
> false, so nothing "automatic" happens unless explicitly turned on.

Done.

https://codereview.chromium.org/598453003/diff/20001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:193: bool autoCompress;
On 2014/09/23 07:24:52, Lasse Reichstein Nielsen wrote:
> If you can enable and disable it, should it still be called "auto", and not
just
> "compress"?
> 
> How about:
>   /**
>    * Whether the [HttpServer] should compress the content, if possible.
>    *
>    * The content can only be compressed when the response is using
>    * chunked Transfer-Encoding and the incoming request has `gzip` in 
>    * as an accepted encoding in the Accept-Encoding header.
>    *
>    * The default value is `true` (compression enabled). 
>    * To disable, set `compress` to false.
>    */
>   bool compress; 

Done.

[Søren Gjesse, 2014-09-23 08:53:54]

lgtm

[Anders Johnsen, 2014-09-23 09:14:50]

PTAL - updated http_multi_server

[Lasse Reichstein Nielsen, 2014-09-23 10:39:59]

lgtm

https://codereview.chromium.org/598453003/diff/80001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/598453003/diff/80001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:188: * The default value is `false` (compression disabled).
You change the default to false. Is this different from the previous behavior if
you don't do anything? If so, it should be a major-version update, not minor.

[Lasse Reichstein Nielsen, 2014-09-23 10:40:01]

lgtm

lgtm

[Anders Johnsen, 2014-09-23 11:08:05]

https://codereview.chromium.org/598453003/diff/80001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/598453003/diff/80001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:188: * The default value is `false` (compression disabled).
On 2014/09/23 10:39:59, Lasse Reichstein Nielsen wrote:
> You change the default to false. Is this different from the previous behavior
if
> you don't do anything? If so, it should be a major-version update, not minor.

This is transparent to the sdk change, so a minor should be fine. (the SDK
restriction should be updated though.)

[Anders Johnsen, 2014-09-23 11:08:36]

Committed patchset #5 (id:80001) manually as 40587 (presubmit successful).

[kevmoo, 2014-09-23 17:14:47]

On 2014/09/23 11:08:36, Anders Johnsen wrote:
> Committed patchset #5 (id:80001) manually as 40587 (presubmit successful).

Do we want to change default here? We're changing the default behavior, right?
Something we should avoid...

[nweiz, 2014-09-23 19:34:45]

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

[nweiz, 2014-09-23 19:34:46]

lgtm

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/te...
File pkg/http_multi_server/test/http_multi_server_test.dart (right):

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/te...
pkg/http_multi_server/test/http_multi_server_test.dart:62: );
Nit: "});"

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/te...
pkg/http_multi_server/test/http_multi_server_test.dart:63: test("autoCompress=
sets the value for all servers", () {
Nit: empty line between tests.

[kevmoo, 2014-09-23 21:03:57]

Must change pubspec constraint on SDK

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/pu...
File pkg/http_multi_server/pubspec.yaml (right):

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/pu...
pkg/http_multi_server/pubspec.yaml:11: sdk: ">=1.6.0-dev.6.0 <2.0.0"
Must update this constraints to be 1.7

[nweiz, 2014-09-23 21:10:33]

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/pu...
File pkg/http_multi_server/pubspec.yaml (right):

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/pu...
pkg/http_multi_server/pubspec.yaml:11: sdk: ">=1.6.0-dev.6.0 <2.0.0"
On 2014/09/23 21:03:57, kevmoo wrote:
> Must update this constraints to be 1.7

I'm planning on doing this in a CL that releases v1.3.0.

[Anders Johnsen, 2014-09-24 09:33:13]

That this was on before, was quite undocumented and for some advance users, very
surprising.

We have to realize the Dart is rarely the front-facing server in a setup, but is
often used behind some kind of reverse proxy. In such setups, using CPU on gzip
is quite wasteful and gives little gain.

Dart comes with batteries included, and I think it's cool we have this built in
to the HTTP stack, but we should not have surprising features enabled by
default.

I've added a line in the release notes about this.

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/te...
File pkg/http_multi_server/test/http_multi_server_test.dart (right):

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/te...
pkg/http_multi_server/test/http_multi_server_test.dart:62: );
On 2014/09/23 19:34:46, nweiz wrote:
> Nit: "});"

Done.

https://codereview.chromium.org/598453003/diff/80001/pkg/http_multi_server/te...
pkg/http_multi_server/test/http_multi_server_test.dart:63: test("autoCompress=
sets the value for all servers", () {
On 2014/09/23 19:34:46, nweiz wrote:
> Nit: empty line between tests.

Done.