Add chunked conversion to converters.

Add chunked conversion to converters.

R=sgjesse@google.com

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

[floitsch, 2013-07-23 16:11:59]

Had to remove the uses of typed data since they don't work in IE9.

[floitsch, 2013-07-23 16:46:18]

Improved some types.

[Søren Gjesse, 2013-07-24 09:26:41]

LGTM

I like the overall concept seen from the user side.

It seems to be somewhat complicated to exactly figure out how little you can get
away with to implement a simple converter and how to turn that into a chunked
converter. I guess this will be a matter of refining the documentation.

Something which I find somewhat confusing the the concept of interfaces. E.g. in
the chunked_conversion1_test.dart test to make a chinked converter one also need
to define additional classes for the interfaces. Would it be possible to just
set these to null when no adaption is available?

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
File sdk/lib/convert/byte_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
sdk/lib/convert/byte_conversion.dart:16: if (sink.interface == _ByteInterface)
return sink;
_ByteInterface => ByteConvertionSink.INTERFACE

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
sdk/lib/convert/byte_conversion.dart:44: _ByteConversionCallbackSink;
How about also having a  ByteConvertionSink.withFuture? The returned sink should
then have a getter for the future (mayne named done).

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
sdk/lib/convert/byte_conversion.dart:119: static int _roundToPowerOf2(int v) {
This is not the first time I have seen this code. Should we add it to Math?

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/chunked_co...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/chunked_co...
sdk/lib/convert/chunked_conversion.dart:38: abstract class
ChunkedConversionSink<T, ChunkedT> {
Consider moving the documentation of T and chunkedT up to the class
documentation.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/converter....
File sdk/lib/convert/converter.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/converter....
sdk/lib/convert/converter.dart:43: ChunkedConversionSink<S, dynamic>
startChunkedConversion(
How about just calling this something shorter? Maybe just start.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/json.dart
File sdk/lib/convert/json.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/json.dart#...
sdk/lib/convert/json.dart:199: StringBuffer buffer = _stringSink;
Any reason for this temporary?

Maybe relying on the _stringSink from super is the StringBuffer passed in the
constructer is a but subtle. But of cause they are both private classes in the
same package.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:232: }  else {
Two spaces before else.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:252: // Otherwise store it in the carry. If [isLast] is
true, then
No need for markup in non-dartdoc comment.

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/chunked_...
File tests/lib/convert/chunked_conversion_utf82_test.dart (right):

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/chunked_...
tests/lib/convert/chunked_conversion_utf82_test.dart:14: bytes.forEach((byte) {
byteSink.add([byte]); });
Consider also testing with adding 2, 3, ... bytes at the time - for all chunked
UTF-8 tests.

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf83_te...
File tests/lib/convert/utf83_test.dart (right):

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf83_te...
tests/lib/convert/utf83_test.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
No need for this change.

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf8_tes...
File tests/lib/convert/utf8_test.dart (right):

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf8_tes...
tests/lib/convert/utf8_test.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
No need for this change.

[floitsch, 2013-07-24 17:59:37]

Removed addNonChunked.
Will now address comments.

[floitsch, 2013-07-24 18:31:15]

PTAL.
I removed addNonChunked and generally simplified a little bit (I hope).

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
File sdk/lib/convert/byte_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
sdk/lib/convert/byte_conversion.dart:16: if (sink.interface == _ByteInterface)
return sink;
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> _ByteInterface => ByteConvertionSink.INTERFACE

Done.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
sdk/lib/convert/byte_conversion.dart:44: _ByteConversionCallbackSink;
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> How about also having a  ByteConvertionSink.withFuture? The returned sink
should
> then have a getter for the future (mayne named done).

Removed that constructor during removal of addNonChunked.
I'm not sure we need the constructor: when working with dart:async then you
probably work with streams, in which case the converter should be used as
transformer.
Will think about it.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/byte_conve...
sdk/lib/convert/byte_conversion.dart:119: static int _roundToPowerOf2(int v) {
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> This is not the first time I have seen this code. Should we add it to Math?
Filed http://dartbug.com/12008.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/chunked_co...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/chunked_co...
sdk/lib/convert/chunked_conversion.dart:38: abstract class
ChunkedConversionSink<T, ChunkedT> {
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> Consider moving the documentation of T and chunkedT up to the class
> documentation.

ChunkedT was removed.
I hope that the single generic type is now self-explanatory.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/converter....
File sdk/lib/convert/converter.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/converter....
sdk/lib/convert/converter.dart:43: ChunkedConversionSink<S, dynamic>
startChunkedConversion(
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> How about just calling this something shorter? Maybe just start.

I'm torn...
Will discuss with others to get a feeling.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/json.dart
File sdk/lib/convert/json.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/json.dart#...
sdk/lib/convert/json.dart:199: StringBuffer buffer = _stringSink;
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> Any reason for this temporary?
> 
> Maybe relying on the _stringSink from super is the StringBuffer passed in the
> constructer is a but subtle. But of cause they are both private classes in the
> same package.

The temporary was to get the StringBuffer type. I wanted to clear the buffer,
but forgot.
now clearing the buffer.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:232: }  else {
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> Two spaces before else.

good eye.
done.

https://codereview.chromium.org/19883003/diff/9001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:252: // Otherwise store it in the carry. If [isLast] is
true, then
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> No need for markup in non-dartdoc comment.

Done.

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/chunked_...
File tests/lib/convert/chunked_conversion_utf82_test.dart (right):

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/chunked_...
tests/lib/convert/chunked_conversion_utf82_test.dart:14: bytes.forEach((byte) {
byteSink.add([byte]); });
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> Consider also testing with adding 2, 3, ... bytes at the time - for all
chunked
> UTF-8 tests.

Done.

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf83_te...
File tests/lib/convert/utf83_test.dart (right):

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf83_te...
tests/lib/convert/utf83_test.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> No need for this change.

Done.

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf8_tes...
File tests/lib/convert/utf8_test.dart (right):

https://codereview.chromium.org/19883003/diff/9001/tests/lib/convert/utf8_tes...
tests/lib/convert/utf8_test.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/07/24 09:26:41, Søren Gjesse wrote:
> No need for this change.

Done.

[Søren Gjesse, 2013-07-25 08:06:59]

lgtm

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
File sdk/lib/convert/byte_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
sdk/lib/convert/byte_conversion.dart:48: * Adds the substring defined by [start]
and [end]-exclusive to `this`.
substring -> bytes

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
sdk/lib/convert/byte_conversion.dart:52: * The given [chunk] is not hold onto.
Once the method returns, it is safe to
hold -> held (I think).

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
sdk/lib/convert/byte_conversion.dart:122: // TODO(11971, floitsch): use
Uint8List instead of normal lists.
We have the class BytesBuilder in dart:io which does this. Maybe we should move
that to dart:typed_data for broader use.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
File sdk/lib/convert/string_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
sdk/lib/convert/string_conversion.dart:36: * partial strings and UTF-8 code
units.
The comment here is a bit misleading. It says "accepts partial strings and UTF-8
code units". However it is more an "or" than "and". It accepts strings and
partial strings or by using asUtf8Sink one can supply UTF-8 code units.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
sdk/lib/convert/string_conversion.dart:67: * This method must be the first and
only call to `this`. It invalidates
Prefix "This meth..." with "If used".

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
sdk/lib/convert/string_conversion.dart:77: * This method must be the first and
only call to `this`. It invalidates
Ditto.

[floitsch, 2013-07-25 12:57:24]

PTAL. Removed ChunkedConversionInterface.
I think we are getting close.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
File sdk/lib/convert/byte_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
sdk/lib/convert/byte_conversion.dart:48: * Adds the substring defined by [start]
and [end]-exclusive to `this`.
On 2013/07/25 08:07:00, Søren Gjesse wrote:
> substring -> bytes

Done.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
sdk/lib/convert/byte_conversion.dart:52: * The given [chunk] is not hold onto.
Once the method returns, it is safe to
On 2013/07/25 08:07:00, Søren Gjesse wrote:
> hold -> held (I think).

Done.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/byte_conv...
sdk/lib/convert/byte_conversion.dart:122: // TODO(11971, floitsch): use
Uint8List instead of normal lists.
On 2013/07/25 08:07:00, Søren Gjesse wrote:
> We have the class BytesBuilder in dart:io which does this. Maybe we should
move
> that to dart:typed_data for broader use.

I think we want to add growable typed lists to typed_data. No reason to limit
the functionality to what is in dart:io.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
File sdk/lib/convert/string_conversion.dart (right):

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
sdk/lib/convert/string_conversion.dart:36: * partial strings and UTF-8 code
units.
On 2013/07/25 08:07:00, Søren Gjesse wrote:
> The comment here is a bit misleading. It says "accepts partial strings and
UTF-8
> code units". However it is more an "or" than "and". It accepts strings and
> partial strings or by using asUtf8Sink one can supply UTF-8 code units.

Rewritten.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
sdk/lib/convert/string_conversion.dart:67: * This method must be the first and
only call to `this`. It invalidates
On 2013/07/25 08:07:00, Søren Gjesse wrote:
> Prefix "This meth..." with "If used".

Done.

https://codereview.chromium.org/19883003/diff/25001/sdk/lib/convert/string_co...
sdk/lib/convert/string_conversion.dart:77: * This method must be the first and
only call to `this`. It invalidates
On 2013/07/25 08:07:00, Søren Gjesse wrote:
> Ditto.

Done.

[Søren Gjesse, 2013-07-25 13:21:04]

LGTM!

[floitsch, 2013-07-25 13:27:19]

Committed patchset #8 manually as r25466 (presubmit successful).