Update the streams interfaces

This contains a new InputStream and StringInputStream interfaces.

The readUntil is only supported through readLine on the StringInputStream.

The StringInputStream implementation in string_stream.dart could live outside the VM as it is plain Dart code.

BUG=
TEST=

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

[Søren Gjesse, 2011-10-17 15:36:41]

This is a request for comments on the update to the streams interfaces. As we
have been discussing offline we will try to move to not exposing the underlying
socket but making the streams interfaces the main interface for communication.

All the methods on input streams which receive date are all passed an object
with the data. This is to ensure that the implementation can implement the data
buffers as efficiently as possible and copying of data can be avoided.

Both types of streams have close and error event handlers. In some situations
(e.g. the Process object) only the streams are available, and the underlying
socket is not exposed.

I have also been thinking that the callbacks could have an additional argument
where errors/close could be reported instead, but I think it is best to keep it
separate.

I am not sure whether the StringInputStream/StringOutputStream interfaces should
be merged with the InputStream/OutputStream as string operations will probably
be commonly used. Another option is to implement string streams as a separate
class which can be chained with the binary stream

  InputStream is = ...
  StringInputStream sis = new StringInputStream(is)

That way all streams don't have to implement string stream support, but
combining string and binary read/write will become more complicated (that might
be rare anyway).

[dcarlson, 2011-10-17 21:05:20]

I'm interested in adding the event emitter code.

This design exposes many low-level details -- important when needed -- but I'd
be interested in a simplistic variation that appears to be the primary flow (see
comments for specifics).

--david

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:11: interface InputStream {
Is there a reason to not have a way to just register for reads? (e.g. just
provide a callback to the InputStream for data events -- in this context, a
readHandler).

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:14: * specified [callback] is called with the
data.
Address EOF case.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:15: void readFully(int len, void
callback(List<int> buffer));
Seems like this is the most common case -- perhaps this could be read, and read
-> something low-level.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:22: */
Also: clarify what happens when you hit EOF prior to a match.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:32: List<int> read(void callback());
make callback optional named param?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:76: String read(void callback());
optional param -- see above.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:82: void set encoding(String encoding);
getter?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart
File runtime/bin/output_stream.dart (right):

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:16: void WriteFully(List<int> buffer, int offset,
int len, void callback())
offset, len, and callback optional?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:16: void WriteFully(List<int> buffer, int offset,
int len, void callback())
Naming question same as InputStream.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:16: void WriteFully(List<int> buffer, int offset,
int len, void callback())
The use of the callback does seem to share the semantics of a Promise. Maybe we
can discuss this face-to-face.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:63: void set encoding(String encoding);
Getter? same as StringInputStream.

[rchandia, 2011-10-17 21:53:46]

In the interest of simplicity I'd propose to trim the interfaces to a single
method for InputStream (read) and OutputStream (write). On top of which more
complex ones can be implemented. For more complex behavior other chained input
streams (BufferedInputStream, LineInputStream, TimedOutputStream, etc.) could
build on top and provide the functionality of readFully, writeFully and
readUntil.

Also, it is too bad that we do not have the ability of creating cheap slices of
Lists and Strings. It would further simplify the interface.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:8: * Multiple input streams can be chained
together to operate collaboratively
This does not seem to allow chaining.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:18: * Reads data from the stream into a buffer
until the specified [pattern]
When I hear pattern I think RegEx'es which is not the case here, right?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:21: * [maxLength] bytes have been received the
error handler will be called.
How can errorHandler recover?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:30: * be called when data becomes available.
What happens when readFully() is followed by read()? In other words, is this the
pattern to follow to read less than what was specified to readFully (perhaps
after a timeout or a close())?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:35: * Close the stream. This will normally also
close the underlying
Does this prevent further reads?

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart
File runtime/bin/output_stream.dart (right):

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:16: void WriteFully(List<int> buffer, int offset,
int len, void callback())
WriteFully -> writeFully

http://codereview.chromium.org/8318009/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:50: void WriteFully(String data, int offset, int
len, void callback())
writeFully

[Søren Gjesse, 2011-10-18 15:23:20]

I have now tried to refactor the InputStream to be more simple and event driven.
Added the StringInputStream and ProcessingInputStream as possible chaining
streams.

I am not sure this is the right split, but I think keeping the InputStream
simpler and using chaining will be more flexible.

We need to cover socket streams, pipe streams from processes and file streams in
the same interface. For files there should be a simple way of just reading the
whole file into one buffer.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:8: * Multiple input streams can be chained
together to operate collaboratively
On 2011/10/17 21:53:46, rchandia wrote:
> This does not seem to allow chaining.

Chaining is now use for StringInputStream.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:11: interface InputStream {
On 2011/10/17 21:05:20, dcarlson wrote:
> Is there a reason to not have a way to just register for reads? (e.g. just
> provide a callback to the InputStream for data events -- in this context, a
> readHandler).

I think it will make sense to have a way of registering a callback for when data
is available, as having the "low-level" read function which might return data
directly or make a callback when data is available can be confusing.

We could then move the readFully and readUntil to a separate class which can be
chained with the "simple" input stream. Having all these different functions
which can do callbacks also provides the problem that whatever function is
called last will get the callback.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:14: * specified [callback] is called with the
data.
On 2011/10/17 21:05:20, dcarlson wrote:
> Address EOF case.

The current handling of EOF is that if [len] characters are not read the
callback is not called.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:15: void readFully(int len, void
callback(List<int> buffer));
On 2011/10/17 21:05:20, dcarlson wrote:
> Seems like this is the most common case -- perhaps this could be read, and
read
> -> something low-level.

I have not tried to split the stream interface and moved the "specialized" reads
out of the simplest interface.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:18: * Reads data from the stream into a buffer
until the specified [pattern]
On 2011/10/17 21:53:46, rchandia wrote:
> When I hear pattern I think RegEx'es which is not the case here, right?

Currently it is simple string matching.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:21: * [maxLength] bytes have been received the
error handler will be called.
On 2011/10/17 21:53:46, rchandia wrote:
> How can errorHandler recover?

Currently not known.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:22: */
On 2011/10/17 21:05:20, dcarlson wrote:
> Also: clarify what happens when you hit EOF prior to a match.

Currently there will be not callback if EOF is hit before match.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:30: * be called when data becomes available.
On 2011/10/17 21:53:46, rchandia wrote:
> What happens when readFully() is followed by read()? In other words, is this
the
> pattern to follow to read less than what was specified to readFully (perhaps
> after a timeout or a close())?

It is a problem with these different functions on the interface as they don't
interact very well. When the interface structure is finalized we will figure out
how they can be combined.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:32: List<int> read(void callback());
On 2011/10/17 21:05:20, dcarlson wrote:
> make callback optional named param?

Removed the callback in favor the dataHandler.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:35: * Close the stream. This will normally also
close the underlying
On 2011/10/17 21:53:46, rchandia wrote:
> Does this prevent further reads?

We should make it possible to read all data after a stream is closed. Maybe
there should only be a close method on an OutputStream. An Input stream can only
receive a close event when it is somehow closed by the other end and all data
have been received.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:76: String read(void callback());
On 2011/10/17 21:05:20, dcarlson wrote:
> optional param -- see above.

Removed the callback instead.

http://codereview.chromium.org/8318009/diff/1/runtime/bin/input_stream.dart#n...
runtime/bin/input_stream.dart:82: void set encoding(String encoding);
On 2011/10/17 21:05:20, dcarlson wrote:
> getter?

Only getter now - removed setter.

[Søren Gjesse, 2011-10-18 15:23:55]

I forgot to mention - no updated to the OutputStream.

[dcarlson, 2011-10-18 17:26:18]

In general, this feels a lot cleaner.

What are you looking for on next steps?

(I don't feel familiar with the use cases the ProcessingInputStream supports, so
I'm staying out of that one.)

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:38: interface StringInputStream factory
_StringInputStream {
I'm surprised that it does not generate close/error events. Is this intended?

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:43: StringInputStream(InputStream input, [String
encoding = "UTF-8"]);
Could we add a dataHandler as an optional param?

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:54: String get encoding();
Can the encoding be changed mid-stream?

[sra1, 2011-10-18 23:45:51]

DBC

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:21: void set dataHandler(void callback());
There is a subtle relationship between the number of times read() may be called
and the number of times the callback is called.  Please specify this completely.

Questions I can't yet answer:

Will calling read() once in the callback always return a non-null value?

If the callback contains two read() calls, can the second call return data or
will the second one always return null?

Can the callback be called multiple times if there is no intervening read()?

Can I rely on the callback always being called so that the read()s are always
inside the callback?

If the InputStream is created and data is immediately available (e.g. a memory
based stream), does the callback get called? If so, during or after the setter?

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:93: void set closeHandler(void callback());
This implies that the underlying's closeHander setter is called, either with the
callback or a wrapped callback.
What happens to pre-existing closeHandler on the underlying?
What happens if input.closeHandler is called after processingInput.closeHandler?

If this is all dangerous behaviour, can it be made to cause an error?

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/output_stream.dart
File runtime/bin/output_stream.dart (right):

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/output_stream.da...
runtime/bin/output_stream.dart:16: void WriteFully(List<int> buffer, int offset,
int len, void callback())
Why is this WriteFully, not writeFully?
Is this a new convention?

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/output_stream.da...
runtime/bin/output_stream.dart:19: * Writes up til [len] bytes from buffer
[buffer] starting at offset
'up til' --> 'up to'

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/output_stream.da...
runtime/bin/output_stream.dart:21: * written the specified callback is called
when more data can be written.
When the callback is called, has the data been written or must it be written
again?

When callbacks are called conditionally, specify the condition precisely.  Here
I think it is 'return != len' but I'm not sure.

[Søren Gjesse, 2011-10-19 14:45:41]

I have now done an initial implementation using the new InputStream interface.
It is called InputStream2 for now to keep the current InputStream until we are
satisfied with the new one.

I have also added a UFT-8 StringInputStream implementation to see how chaining
streams will work.

The InputStream2 interface is used for the Process tests.

For the Process tests I am currently looking into why the current implementation
does not provide a close event on the  stdout and strerr streams when the
process exits.

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:21: void set dataHandler(void callback());
On 2011/10/18 23:45:51, sra1 wrote:
> There is a subtle relationship between the number of times read() may be
called
> and the number of times the callback is called.  Please specify this
completely.
> 
> Questions I can't yet answer:
> 
> Will calling read() once in the callback always return a non-null value?

That should hopefully be the case - unless the OS somehow lies to us.
> 
> If the callback contains two read() calls, can the second call return data or
> will the second one always return null?

It can return data as new data might have arrived.

> 
> Can the callback be called multiple times if there is no intervening read()?

Most likely not.

> 
> Can I rely on the callback always being called so that the read()s are always
> inside the callback?

Yes.

> 
> If the InputStream is created and data is immediately available (e.g. a memory
> based stream), does the callback get called? If so, during or after the
setter?

We should ensure that hte callback is called and always called from the event
loop not directly by the setter. The setter should then schedule the event.

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:38: interface StringInputStream factory
_StringInputStream {
On 2011/10/18 17:26:19, dcarlson wrote:
> I'm surprised that it does not generate close/error events. Is this intended? 

No, it should also generate the close and error events.

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:43: StringInputStream(InputStream input, [String
encoding = "UTF-8"]);
On 2011/10/18 17:26:19, dcarlson wrote:
> Could we add a dataHandler as an optional param?

Maybe, but then we should also do that for the InputStream as well. With named
optional arguments we could consider it.

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:54: String get encoding();
On 2011/10/18 17:26:19, dcarlson wrote:
> Can the encoding be changed mid-stream?

No.

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/input_stream.dar...
runtime/bin/input_stream.dart:93: void set closeHandler(void callback());
On 2011/10/18 23:45:51, sra1 wrote:
> This implies that the underlying's closeHander setter is called, either with
the
> callback or a wrapped callback.
> What happens to pre-existing closeHandler on the underlying?
> What happens if input.closeHandler is called after
processingInput.closeHandler?
> 
> If this is all dangerous behaviour, can it be made to cause an error?

My take on this is that if you wrap an InputStream in a StringInputStream (or
some other stream) you should not touch the stream wrapped. This also goes for
calling read on the wrapped stream. I am not sure we can enforce that though.

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/output_stream.dart
File runtime/bin/output_stream.dart (right):

http://codereview.chromium.org/8318009/diff/5001/runtime/bin/output_stream.da...
runtime/bin/output_stream.dart:16: void WriteFully(List<int> buffer, int offset,
int len, void callback())
On 2011/10/18 23:45:51, sra1 wrote:
> Why is this WriteFully, not writeFully?
> Is this a new convention?

No, that is a mistake.

[Søren Gjesse, 2011-10-20 11:43:39]

I discussed the InputStream2 interface with iposva@ this morning, and he was
concerned by the read method just delivering whatever data is available. This
could be a lot of data and there is no way of controlling how much is delivered
with this API.

I have updated InputStream2 based on this feedback.

1. I have added an optional argument to the read call to limit the number of
bytes read. I still think that just reading all data is practical e.g. getting
an input stream for a file and just reading it into memory.

2. I also added a readInto method which is convenient if one wants to copy the
data into another buffer.

3. A getter for the available number of bytes have also been added.

4. Finally I have also added a separate way of setting the data handler where
the minimum number of bytes of interest can be specified. We cannot use the
setter syntax then, and I don't know whether that will be useful. Also if the
stream is closed before this number of bytes have been received should the
callback still be called with the lesser amount of data or should the close
handler be called directly?

[dcarlson, 2011-10-20 14:47:43]

WRT chaining: I would like to follow up with some exploration of chaining. I
think we want to do things like decouple a StringInputStream from an underlying
IS for things like dealing with differing encodings on multipart messages.

> 2. I also added a readInto method which is convenient if one wants to copy the
> data into another buffer.

Not tied to moving this CL forward, I think we could benefit from a ByteBuffer
type. The ability to defer/avoid copying/converting data into Dart types could
reduce the amount of copying.

Otherwise, I am good regarding the design of InputStream2.

[rchandia, 2011-10-20 15:47:19]

InputStream is missing a 'closed' getter to support the operation without
callbacks.

[rchandia, 2011-10-20 15:49:33]

InputStream2, I mean.

[rchandia, 2011-10-20 15:49:51]

http://codereview.chromium.org/8318009/diff/11001/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/8318009/diff/11001/runtime/bin/process.dart#ne...
runtime/bin/process.dart:24: InputStream2 get stdoutStream();
Can we rename this to stdout?

http://codereview.chromium.org/8318009/diff/11001/runtime/bin/process.dart#ne...
runtime/bin/process.dart:29: InputStream2 get stderrStream();
stderr?

[Søren Gjesse, 2011-10-21 15:16:50]

On 2011/10/20 14:47:43, dcarlson wrote:
> WRT chaining: I would like to follow up with some exploration of chaining. I
> think we want to do things like decouple a StringInputStream from an
underlying
> IS for things like dealing with differing encodings on multipart messages.
> 

I agree that just having the ability to turn an InputStream into a
StringInputStream will be to simple for many applications. Maybe we should look
at exposing stream decoders, with an interface like I have for the UTF8Decoder.
You push in bytes in one end and draw something lese in the other end, e.g. a
string. This is how the HTTP parser handles chunked encoding being decoded to a
string. A JSON parser can be written in the same way where string pieces pulled
from one decoder are pushed into the JSON decoder and in the end you can draw a
map from the JSON decoder.

> > 2. I also added a readInto method which is convenient if one wants to copy
the
> > data into another buffer.
> 
> Not tied to moving this CL forward, I think we could benefit from a ByteBuffer
> type. The ability to defer/avoid copying/converting data into Dart types could
> reduce the amount of copying.
> 

I discussed this with iposva@ the other day, and we could have buffers  that
could be backed by external storage. However this can conflict with normal GC
policies, as the objects inside the VM holding the external storage will be
quite small and therefore with normal GC policies would not cause a GC so a lot
of external storage will be reclaimed much to late. Anyway if we move to another
type of buffers they could still implement the List interface and would fit with
the interface.

> Otherwise, I am good regarding the design of InputStream2.

[Søren Gjesse, 2011-10-21 15:17:03]

http://codereview.chromium.org/8318009/diff/11001/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/8318009/diff/11001/runtime/bin/process.dart#ne...
runtime/bin/process.dart:24: InputStream2 get stdoutStream();
On 2011/10/20 15:49:51, rchandia wrote:
> Can we rename this to stdout?

Sounds resonable. I will make a separate cl with the current input stream.

http://codereview.chromium.org/8318009/diff/11001/runtime/bin/process.dart#ne...
runtime/bin/process.dart:29: InputStream2 get stderrStream();
On 2011/10/20 15:49:51, rchandia wrote:
> stderr?

Ditto.

[Mads Ager (google), 2011-10-26 10:56:07]

LGTM

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:49: void set dataHandler(void callback()) {
I guess a dataHandler callback is scheduled immediately if you have enough data.
Then when you return, either a new one is scheduled if you have more data or the
endHandler is called?

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:53: void set closeHandler(void callback()) {
Maybe call this end instead. That would make sense for both sockets and files.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:40: * Sets the data handler gets called when data
is available.
get called -> which gets called

As a separate changelist we should add a chunk size so we can control that the
dataHandler only gets invoked at end or if at least a chunk size of data is
available.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:45: * The close handler gets called when the
underlying communication
As discussed offline we should rename to endHandler.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:75: * available null will be returned..
.. -> .

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:83: bool get closed();
If we rename closeHandler to endHandler maybe this should be renamed to ended?

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:115: * Contains the exception message.
Remove comment. Doesn't add much.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.dart
File runtime/bin/string_stream.dart (right):

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:39: // Add more binary data to be decoded.
Ownership of the buffer is transferred to the Decoder, right? So people should
not reuse the buffer when this returns? If so, we might want to add a comment to
that effect.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:45: // Get the decoded data.
Get the data decoded since the last call to decoded.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:81: // Return the string decoded so far.
so far -> since the last call to decoded.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:150: // Check if there are not enough bytes to
decode the character
Check if there are enough bytes to decode the character. Otherwise return false.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:241: print("close handler");
Remove

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:292: // Fill decoded data into the buffer Returns
true if more data was
Period after 'buffer'.

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/Proces...
File tests/standalone/src/ProcessStdoutTest.dart (right):

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/Proces...
tests/standalone/src/ProcessStdoutTest.dart:35: print(buffer.length);
Remove?

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
File tests/standalone/src/StringStreamTest.dart (right):

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
tests/standalone/src/StringStreamTest.dart:33: print("Closing");
Remove?

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
tests/standalone/src/StringStreamTest.dart:114: 
remove blank line?

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
tests/standalone/src/StringStreamTest.dart:147: 
Remove the blank lines?

[Søren Gjesse, 2011-10-26 12:03:31]

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:49: void set dataHandler(void callback()) {
On 2011/10/26 10:56:07, Mads Ager wrote:
> I guess a dataHandler callback is scheduled immediately if you have enough
data.
> Then when you return, either a new one is scheduled if you have more data or
the
> endHandler is called?

I will address this in a separate cl.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:53: void set closeHandler(void callback()) {
On 2011/10/26 10:56:07, Mads Ager wrote:
> Maybe call this end instead. That would make sense for both sockets and files.

Will change from close to end in a separate cl.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.dart
File runtime/bin/input_stream.dart (right):

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:40: * Sets the data handler gets called when data
is available.
On 2011/10/26 10:56:07, Mads Ager wrote:
> get called -> which gets called
> 
> As a separate changelist we should add a chunk size so we can control that the
> dataHandler only gets invoked at end or if at least a chunk size of data is
> available. 

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:45: * The close handler gets called when the
underlying communication
On 2011/10/26 10:56:07, Mads Ager wrote:
> As discussed offline we should rename to endHandler.

Will do in a separate cl.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:75: * available null will be returned..
On 2011/10/26 10:56:07, Mads Ager wrote:
> .. -> .

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:83: bool get closed();
On 2011/10/26 10:56:07, Mads Ager wrote:
> If we rename closeHandler to endHandler maybe this should be renamed to ended?

Separate cl. Probably ended or maybe isEnded.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/input_stream.da...
runtime/bin/input_stream.dart:115: * Contains the exception message.
On 2011/10/26 10:56:07, Mads Ager wrote:
> Remove comment. Doesn't add much.

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.dart
File runtime/bin/string_stream.dart (right):

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:39: // Add more binary data to be decoded.
On 2011/10/26 10:56:07, Mads Ager wrote:
> Ownership of the buffer is transferred to the Decoder, right? So people should
> not reuse the buffer when this returns? If so, we might want to add a comment
to
> that effect.

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:45: // Get the decoded data.
On 2011/10/26 10:56:07, Mads Ager wrote:
> Get the data decoded since the last call to decoded.

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:81: // Return the string decoded so far.
On 2011/10/26 10:56:07, Mads Ager wrote:
> so far -> since the last call to decoded.

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:150: // Check if there are not enough bytes to
decode the character
On 2011/10/26 10:56:07, Mads Ager wrote:
> Check if there are enough bytes to decode the character. Otherwise return
false.

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:241: print("close handler");
On 2011/10/26 10:56:07, Mads Ager wrote:
> Remove

Done.

http://codereview.chromium.org/8318009/diff/27024/runtime/bin/string_stream.d...
runtime/bin/string_stream.dart:292: // Fill decoded data into the buffer Returns
true if more data was
On 2011/10/26 10:56:07, Mads Ager wrote:
> Period after 'buffer'.

Done.

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/Proces...
File tests/standalone/src/ProcessStdoutTest.dart (right):

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/Proces...
tests/standalone/src/ProcessStdoutTest.dart:35: print(buffer.length);
On 2011/10/26 10:56:07, Mads Ager wrote:
> Remove?

Done.

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
File tests/standalone/src/StringStreamTest.dart (right):

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
tests/standalone/src/StringStreamTest.dart:33: print("Closing");
On 2011/10/26 10:56:07, Mads Ager wrote:
> Remove?

Done.

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
tests/standalone/src/StringStreamTest.dart:114: 
On 2011/10/26 10:56:07, Mads Ager wrote:
> remove blank line?

Done.

http://codereview.chromium.org/8318009/diff/27024/tests/standalone/src/String...
tests/standalone/src/StringStreamTest.dart:147: 
On 2011/10/26 10:56:07, Mads Ager wrote:
> Remove the blank lines?

Done.