Introduce (private) ZLibFilter to dart:io.

Introduce (private) ZLibFilter to dart:io.

This is to be used in gzip to handle http compression.

BUG=

[Søren Gjesse, 2012-10-25 11:01:44]

Some additional comments. Looks good, but it needs some tests before landing.

Also add third_party/zlib to the LICENSE file.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:63: Dart_Handle err =
DartUtils::NewDartOSError(&os_error);
I am sorry for leading you down this path, but the errors here are not really OS
errors. We should reserve OS errors for errors returned from "real" OS calls.

We should probably add an InternalError for these kind of errors. You can leave
it as a TODO and open a bug on it.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:82: delete[] data;
Maybe use DeleteBuffer here (and in the other places) to be somewhat consistent.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:125: } else {
Can't you create an external byte array here as well? I don't see any problem of
the buffer being larger than indicated by length. Maybe for very small values of
length you should still fallback to byte arrays in the Dart heap.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:175: kZLibFlagWindowBits + (gZip ? kZLibFlagUseGZipHeader
: 0),
Maybe | here instead of +

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:226: kZLibFlagWindowBits + kZLibFlagAcceptAnyHeader) ==
Z_OK;
Maybe | here instead of +

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.dart
File runtime/bin/filter.dart (right):

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.dart#ne...
runtime/bin/filter.dart:18: List<int> processed([bool flush = true]);
Maybe named argument instead of positional.

[Anders Johnsen, 2012-10-25 13:10:56]

PTAL, I've added an InternalError as suggested. I'll add tests through
HttpServer in the next round, as the filter is going private now.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:63: Dart_Handle err =
DartUtils::NewDartOSError(&os_error);
On 2012/10/25 11:01:44, Søren Gjesse wrote:
> I am sorry for leading you down this path, but the errors here are not really
OS
> errors. We should reserve OS errors for errors returned from "real" OS calls.
> 
> We should probably add an InternalError for these kind of errors. You can
leave
> it as a TODO and open a bug on it.

I've added an InternalError class and used it accordingly.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:82: delete[] data;
On 2012/10/25 11:01:44, Søren Gjesse wrote:
> Maybe use DeleteBuffer here (and in the other places) to be somewhat
consistent.

Done.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:125: } else {
On 2012/10/25 11:01:44, Søren Gjesse wrote:
> Can't you create an external byte array here as well? I don't see any problem
of
> the buffer being larger than indicated by length. Maybe for very small values
of
> length you should still fallback to byte arrays in the Dart heap.

I like this idea! We are currently using buffers with sizes of 64k. That should
be okay. One concern: The GC does not know how much data we actually have
allocated. Should we add a TODO about adding an extra optional parameter to
NewExternalByteArray with the 'real size'?

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:175: kZLibFlagWindowBits + (gZip ? kZLibFlagUseGZipHeader
: 0),
On 2012/10/25 11:01:44, Søren Gjesse wrote:
> Maybe | here instead of +

Done.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:226: kZLibFlagWindowBits + kZLibFlagAcceptAnyHeader) ==
Z_OK;
On 2012/10/25 11:01:44, Søren Gjesse wrote:
> Maybe | here instead of +

Done.

[Søren Gjesse, 2012-10-25 13:54:45]

LGTM, feel free to land when it builds on all platforms.

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
runtime/bin/filter.cc:125: } else {
On 2012/10/25 13:10:56, ajohnsen wrote:
> On 2012/10/25 11:01:44, Søren Gjesse wrote:
> > Can't you create an external byte array here as well? I don't see any
problem
> of
> > the buffer being larger than indicated by length. Maybe for very small
values
> of
> > length you should still fallback to byte arrays in the Dart heap.
> 
> I like this idea! We are currently using buffers with sizes of 64k. That
should
> be okay. One concern: The GC does not know how much data we actually have
> allocated. Should we add a TODO about adding an extra optional parameter to
> NewExternalByteArray with the 'real size'?

At some point we probable need to give the VM some idea of how much external
data is actually retained, but for now we can ignore it.

https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/L...
File runtime/third_party/zlib/LICENSE (right):

https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/L...
runtime/third_party/zlib/LICENSE:1: /* zlib.h -- interface of the 'zlib' general
purpose compression library
I think you also need to update the LICENSE file in the repository root
(http://code.google.com/p/dart/source/browse/branches/bleeding_edge/LICENSE).

https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/z...
File runtime/third_party/zlib/zlib.gypi (right):

https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/z...
runtime/third_party/zlib/zlib.gypi:1: # Copyright 2010 Google Inc. All Rights
Reserved.
Year 2012

https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/z...
runtime/third_party/zlib/zlib.gypi:1: # Copyright 2010 Google Inc. All Rights
Reserved.
I looked briefly at the Chromium gyp file for zlib at:
http://src.chromium.org/viewvc/chrome/trunk/src/third_party/zlib/zlib.gyp?rev....

Looks like there might be some special settings needed for Mac OS and Windows.

[Søren Gjesse, 2013-03-04 08:23:57]

On 2012/10/25 13:54:45, Søren Gjesse wrote:
> LGTM, feel free to land when it builds on all platforms.
> 
> https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc
> File runtime/bin/filter.cc (right):
> 
>
https://codereview.chromium.org/11275035/diff/2001/runtime/bin/filter.cc#newc...
> runtime/bin/filter.cc:125: } else {
> On 2012/10/25 13:10:56, ajohnsen wrote:
> > On 2012/10/25 11:01:44, Søren Gjesse wrote:
> > > Can't you create an external byte array here as well? I don't see any
> problem
> > of
> > > the buffer being larger than indicated by length. Maybe for very small
> values
> > of
> > > length you should still fallback to byte arrays in the Dart heap.
> > 
> > I like this idea! We are currently using buffers with sizes of 64k. That
> should
> > be okay. One concern: The GC does not know how much data we actually have
> > allocated. Should we add a TODO about adding an extra optional parameter to
> > NewExternalByteArray with the 'real size'?
> 
> At some point we probable need to give the VM some idea of how much external
> data is actually retained, but for now we can ignore it.
> 
>
https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/L...
> File runtime/third_party/zlib/LICENSE (right):
> 
>
https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/L...
> runtime/third_party/zlib/LICENSE:1: /* zlib.h -- interface of the 'zlib'
general
> purpose compression library
> I think you also need to update the LICENSE file in the repository root
> (http://code.google.com/p/dart/source/browse/branches/bleeding_edge/LICENSE).
> 
>
https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/z...
> File runtime/third_party/zlib/zlib.gypi (right):
> 
>
https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/z...
> runtime/third_party/zlib/zlib.gypi:1: # Copyright 2010 Google Inc. All Rights
> Reserved.
> Year 2012
> 
>
https://codereview.chromium.org/11275035/diff/8001/runtime/third_party/zlib/z...
> runtime/third_party/zlib/zlib.gypi:1: # Copyright 2010 Google Inc. All Rights
> Reserved.
> I looked briefly at the Chromium gyp file for zlib at:
>
http://src.chromium.org/viewvc/chrome/trunk/src/third_party/zlib/zlib.gyp?rev....
> 
> Looks like there might be some special settings needed for Mac OS and Windows.

This CL seems out of date, please close.