Redo StreamTransformers so they work with Stack traces.

Redo StreamTransformers so they work with Stack traces.

Since this involved a breaking change I cleaned up more of the
StreamTransformer API.

This CL also deprecates the getAttachedStackTrace method.

R=ajohnsen@google.com, lrn@google.com, nweiz@google.com, rnystrom@google.com

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

[floitsch, 2013-10-04 11:46:07]

Still WIP, but it would be great if you could start the review.

[floitsch, 2013-10-04 12:19:00]

Fixed standalone tests.

[floitsch, 2013-10-04 12:19:38]

@Anders: please have a look at the io libraries and tell me what you think.

[floitsch, 2013-10-04 12:30:14]

Fixed most async tests. Remaining failing test: stream_transformer_test.

[Lasse Reichstein Nielsen, 2013-10-04 14:35:53]

LGTM so fart.
Examples would be great :)

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/_collection_dev/i...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:28: //    getAttachedStackTrace.
Huzzah!

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:178: *     class DuplicationTransformer implements
StreamTransformer<String, String> {
Long line.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:47: : super(onData, onError, onDone,
cancelOnError) {
Indent by four only.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:48: _sink = new
_EventSinkAdapter<T>(this);
I don't think _sink is used anywhere else, so could it just be a local variable
here instead of a field.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:126: * It contains the bound Stream and
the sink-mapper. Only, when the user starts
Only when the user starts listening to this stream is the sink-mapper invoked.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:155: * Wraps handlers (from
[StreamTransformer.fromHandlers]) into an EventSink.
`EventSink`

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:210: static void
_defaultHandleError(error, StackTrace stackTrace, EventSink sink) {
long line.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:225: * This class is the most powerful
[StreamTransformer].
powerful -> generic?
"the most" sounds like it should be quantified. How about a fully generic stream
transformer, or an unrestricted one.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:228: * StreamSubscription. This allows it
to do everything it needs. It can also
do everything it needs -> interact with the entire stream life cycle.
Or something else, but the class doesn't have needs.

[Anders Johnsen, 2013-10-04 15:02:07]

IO LGTM.

[floitsch, 2013-10-05 18:47:39]

@Lasse: PTAL. I have implemented everything I wanted.

Looping in Bob and Nathan for the changes in their packages/libraries.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:178: *     class DuplicationTransformer implements
StreamTransformer<String, String> {
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> Long line.

I know, but since the actual code is less than 80 chars I kept it this way. This
is documentation and there is no reason to cut it in the output.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:47: : super(onData, onError, onDone,
cancelOnError) {
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> Indent by four only.

Done.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:48: _sink = new
_EventSinkAdapter<T>(this);
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> I don't think _sink is used anywhere else, so could it just be a local
variable
> here instead of a field.

Done.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:126: * It contains the bound Stream and
the sink-mapper. Only, when the user starts
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> Only when the user starts listening to this stream is the sink-mapper invoked.

Done.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:155: * Wraps handlers (from
[StreamTransformer.fromHandlers]) into an EventSink.
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> `EventSink`

Done.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:210: static void
_defaultHandleError(error, StackTrace stackTrace, EventSink sink) {
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> long line.

Done.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:225: * This class is the most powerful
[StreamTransformer].
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> powerful -> generic?
> "the most" sounds like it should be quantified. How about a fully generic
stream
> transformer, or an unrestricted one.

"fully generic".
done.

https://codereview.chromium.org/25354003/diff/12001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:228: * StreamSubscription. This allows it
to do everything it needs. It can also
On 2013/10/04 14:35:53, Lasse Reichstein Nielsen wrote:
> do everything it needs -> interact with the entire stream life cycle.
> Or something else, but the class doesn't have needs.

Completely rewrote the comment.

[floitsch, 2013-10-05 20:28:04]

Note: the current implementation triggers a bug in dart2js which makes lots of
tests fail. I will talk to the dart2js devs asap.

[Lasse Reichstein Nielsen, 2013-10-07 11:46:59]

LGTM

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1078: * It is good practice to create transformers
that can be used multiple times.
By "create", do you mean that it is good practice to write transformers so that
they can be reused, or to create and cache reusable transformers.

If the former, replace "create" with "write".

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1087: * the bound stream. At that point the
transformer should start listening
User listens on the returned stream, not the bound stream?

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1089: * [StreamSubscription]. The bound
stream-transformer then sets the handlers
How is the stream-transformer bound, and to what? (I.e., too many "bound" for me
to distinguish what is going on).

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1090: * it received as part of the `listen` call.
Missing handlers (or the ones
Sounds like the handlers, which were received as part of the listen call, are
set (and it's not obvious what that means).

Is this what is happening in general:

- This StreamTransformer is based on a Stream->StreamSubscription transformation
function.
- When an input Stream is [Stream.transform]'ed using this transformation, a new
output Stream is returned.
- When the output stream is listened on, the transformation function is called
on the input stream.
- The subscription returned by the transformation function is used as the result
of the listen call on the output stream. Provided handlers are set on this
subscription.
- Events from the input stream are transformed by the transformation, and the
resulting events become the events on the output stream subscription.

If so, I understand it correctly. The documentation is still confusing.

Maybe use more paragraphs?

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1091: * that are `null`) are also set (with `null`).
Last sentence seems superflous.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1121: *             sync: true);  // One of the few
places, where sync is correct.
Either remove the comment or elaborate on why it is correct (this is pure
forwarding of another event, so it already happens at a place where events are
accepted).

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1132: * This constructor returns a transformer that
can only be used once.
Why can it only be used once? If the handlers themselves are state-less, I think
it should be possible to make it so that you can reuse it.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:10: class _EventSinkAdapter<T> implements
EventSink<T> {
Just to be pedantic (with myself, since I named this), this is not really an
Adapter (q.v. Adapter Design Pattern) since it has the same interface as the
wrapped object. It should probably just be a "Wrapper". It's not complex enough
to warrant being a "Proxy".

Yey for names.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:242: _hasBeenUsed = true;
This was the reason it could only be used once? 
Could we remove it?

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:272: * actual work), this class only
requires a closure that is invoked when the
closure -> function.
Using "closure" suggests that it must be closing over something. Just say
"function" in all cases where you would use "closure", and it will still mean
the same, and it matches with objects having the type Function.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:275: * The given transformer closure maps
from Stream, cancelOnError to a
"Stream, cancelOnError" -> "Stream and cancelOnError"

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:276: * StreamSubscription. As such it can
also act on `cancel` events, making it
`StreamSubscription`.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:277: * fully generic.
"fully generic" -> "fully general".
"Generic" sounds too much like it has something to do with type parameters. My
bad (I know I suggested it) - it sounded great in my head, but seeing it in a
sentence doesn't work.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:285: => new _BoundSubscriptionStream<S,
T>(stream, _transformer);
Style guide wants '=>' on the previous line.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:289: * The bound version of
[_StreamSubscriptionTransformer].
Not obvious what "bound" means. I guess it refers to the transformer's "bind"
method having been called.

How about:
 "A stream transformed by a [_StreamSubscriptionTransformer]."
?

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:291: * This class is a [Stream] that has
already an input stream. The transformer
"has already" -> "already has". Maybe drop "already".

How about:
 "Starts listening to the input stream and transforming its events when this
stream is listened to."
?

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:292: * closure is, however, only invoked
when this stream is listened to.
Drop ", however,". It suggests that this is surprising, when it's just
describing what is happening (I didn't have a prior expectation that this wasn't
following).

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/io/websocket_impl...
File sdk/lib/io/websocket_impl.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/io/websocket_impl...
sdk/lib/io/websocket_impl.dart:473: throw new StateError("WebSocket transformer
already used");
If it didn't try to put everything into one class, it would be reusable.

https://codereview.chromium.org/25354003/diff/28001/tests/lib/async/catch_err...
File tests/lib/async/catch_errors16_test.dart (right):

https://codereview.chromium.org/25354003/diff/28001/tests/lib/async/catch_err...
tests/lib/async/catch_errors16_test.dart:23: handleError: (e, st, sink) =>
sink.add("error $e")))
I prefer braces to '=>' for void functions.

[Lasse Reichstein Nielsen, 2013-10-07 12:32:24]

LGTM

[nweiz, 2013-10-07 20:29:19]

One change, otherwise pub/pkg changes lgtm.

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
File pkg/barback/test/stream_pool_test.dart (right):

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
pkg/barback/test/stream_pool_test.dart:55: handleError: (e, st, sink) =>
sink.add(["error", e]))).toList();
Please use a full word rather than "st". Also below.

[Bob Nystrom, 2013-10-07 20:29:53]

A couple of name changes, then pub stuff LGTM.

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
File pkg/barback/test/stream_pool_test.dart (right):

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
pkg/barback/test/stream_pool_test.dart:55: handleError: (e, st, sink) =>
sink.add(["error", e]))).toList();
e -> error
st -> stackTrace

Here and below.

[Lasse Reichstein Nielsen, 2013-10-08 08:10:55]

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/utf/utf_stream.dart
File sdk/lib/utf/utf_stream.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/utf/utf_stream.da...
sdk/lib/utf/utf_stream.dart:173: implements StreamTransformer<String,
List<int>>, EventSink<String> {
This should really be two classes.

It's not possible to use it as a sink before it's bound, and it's not allowed to
use as a transformer after it's bound. It should just be a transformer that
returns a new sink each time, and then it would work more than once too.

[floitsch, 2013-10-10 15:39:56]

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
File pkg/barback/test/stream_pool_test.dart (right):

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
pkg/barback/test/stream_pool_test.dart:55: handleError: (e, st, sink) =>
sink.add(["error", e]))).toList();
On 2013/10/07 20:29:54, Bob Nystrom wrote:
> e -> error
> st -> stackTrace
> 
> Here and below.

Done.

https://codereview.chromium.org/25354003/diff/28001/pkg/barback/test/stream_p...
pkg/barback/test/stream_pool_test.dart:55: handleError: (e, st, sink) =>
sink.add(["error", e]))).toList();
On 2013/10/07 20:29:20, nweiz wrote:
> Please use a full word rather than "st". Also below.

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1078: * It is good practice to create transformers
that can be used multiple times.
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> By "create", do you mean that it is good practice to write transformers so
that
> they can be reused, or to create and cache reusable transformers.
> 
> If the former, replace "create" with "write".

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1087: * the bound stream. At that point the
transformer should start listening
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> User listens on the returned stream, not the bound stream?

rewritten.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1089: * [StreamSubscription]. The bound
stream-transformer then sets the handlers
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> How is the stream-transformer bound, and to what? (I.e., too many "bound" for
me
> to distinguish what is going on).

rewritten.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1090: * it received as part of the `listen` call.
Missing handlers (or the ones
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Sounds like the handlers, which were received as part of the listen call, are
> set (and it's not obvious what that means).
> 
> Is this what is happening in general:
> 
> - This StreamTransformer is based on a Stream->StreamSubscription
transformation
> function.
> - When an input Stream is [Stream.transform]'ed using this transformation, a
new
> output Stream is returned.
> - When the output stream is listened on, the transformation function is called
> on the input stream.
> - The subscription returned by the transformation function is used as the
result
> of the listen call on the output stream. Provided handlers are set on this
> subscription.
> - Events from the input stream are transformed by the transformation, and the
> resulting events become the events on the output stream subscription.
> 
> If so, I understand it correctly. The documentation is still confusing.
> 
> Maybe use more paragraphs?
> 

rewritten.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1091: * that are `null`) are also set (with `null`).
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Last sentence seems superflous.

It was meant to say that the Subscription can rely on setting the handlers with
"onData", "onError", ... All of them will be invoked.
But it's not that important and I removed it.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1121: *             sync: true);  // One of the few
places, where sync is correct.
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Either remove the comment or elaborate on why it is correct (this is pure
> forwarding of another event, so it already happens at a place where events are
> accepted).

Removed.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1132: * This constructor returns a transformer that
can only be used once.
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Why can it only be used once? If the handlers themselves are state-less, I
think
> it should be possible to make it so that you can reuse it.

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:10: class _EventSinkAdapter<T> implements
EventSink<T> {
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Just to be pedantic (with myself, since I named this), this is not really an
> Adapter (q.v. Adapter Design Pattern) since it has the same interface as the
> wrapped object. It should probably just be a "Wrapper". It's not complex
enough
> to warrant being a "Proxy".
> 
> Yey for names.

Renamed to _EventSinkWrapper. Yey for dartEditor's rename functionality.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:242: _hasBeenUsed = true;
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> This was the reason it could only be used once? 
> Could we remove it?

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:272: * actual work), this class only
requires a closure that is invoked when the
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> closure -> function.
> Using "closure" suggests that it must be closing over something. Just say
> "function" in all cases where you would use "closure", and it will still mean
> the same, and it matches with objects having the type Function.

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:275: * The given transformer closure maps
from Stream, cancelOnError to a
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> "Stream, cancelOnError" -> "Stream and cancelOnError"

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:276: * StreamSubscription. As such it can
also act on `cancel` events, making it
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> `StreamSubscription`.

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:277: * fully generic.
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> "fully generic" -> "fully general".
> "Generic" sounds too much like it has something to do with type parameters. My
> bad (I know I suggested it) - it sounded great in my head, but seeing it in a
> sentence doesn't work.

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:285: => new _BoundSubscriptionStream<S,
T>(stream, _transformer);
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Style guide wants '=>' on the previous line.

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:289: * The bound version of
[_StreamSubscriptionTransformer].
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Not obvious what "bound" means. I guess it refers to the transformer's "bind"
> method having been called.
> 
> How about:
>  "A stream transformed by a [_StreamSubscriptionTransformer]."
> ?

Done.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:291: * This class is a [Stream] that has
already an input stream. The transformer
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> "has already" -> "already has". Maybe drop "already".
> 
> How about:
>  "Starts listening to the input stream and transforming its events when this
> stream is listened to."
> ?

Rewritten.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/async/stream_tran...
sdk/lib/async/stream_transformers.dart:292: * closure is, however, only invoked
when this stream is listened to.
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> Drop ", however,". It suggests that this is surprising, when it's just
> describing what is happening (I didn't have a prior expectation that this
wasn't
> following).

Rewritten.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/io/websocket_impl...
File sdk/lib/io/websocket_impl.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/io/websocket_impl...
sdk/lib/io/websocket_impl.dart:473: throw new StateError("WebSocket transformer
already used");
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> If it didn't try to put everything into one class, it would be reusable.

Agreed.
Added a TODO. I just wanted to keep the changes small.

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/utf/utf_stream.dart
File sdk/lib/utf/utf_stream.dart (right):

https://codereview.chromium.org/25354003/diff/28001/sdk/lib/utf/utf_stream.da...
sdk/lib/utf/utf_stream.dart:173: implements StreamTransformer<String,
List<int>>, EventSink<String> {
On 2013/10/08 08:10:56, Lasse Reichstein Nielsen wrote:
> This should really be two classes.
> 
> It's not possible to use it as a sink before it's bound, and it's not allowed
to
> use as a transformer after it's bound. It should just be a transformer that
> returns a new sink each time, and then it would work more than once too.

Agreed. But since this is in the deprecated UTF library it should not matter. I
added a TODO in the utf package.

https://codereview.chromium.org/25354003/diff/28001/tests/lib/async/catch_err...
File tests/lib/async/catch_errors16_test.dart (right):

https://codereview.chromium.org/25354003/diff/28001/tests/lib/async/catch_err...
tests/lib/async/catch_errors16_test.dart:23: handleError: (e, st, sink) =>
sink.add("error $e")))
On 2013/10/07 11:47:00, Lasse Reichstein Nielsen wrote:
> I prefer braces to '=>' for void functions.

Done.

[floitsch, 2013-10-12 12:35:08]

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