Make IOSink implement StringSink

Make IOSink implement StringSink

Besides adding the StringSink methods I also added writeBytes and
deprecated both add and addString.

To handle the encoding of strings the IOSike has an encoding
property. This property is mutable in situation when it makes sense to
change encoding of what is written. The exception here is for HTTP
where the encoding is determined from the header and the encoding
cannot be changed.

R=ajohnsen@google.com, ager@google.com, nweiz@google.com

BUG=

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

[Søren Gjesse, 2013-03-06 15:29:31]

Mads and Anders please review this.

Nathan please take a look at the pkg/http changes.

This is based on https://codereview.chromium.org/12440002/ which is not
committed yet.

We should also consider renaming addStream to writeStream now.

[nweiz, 2013-03-06 20:31:51]

https://codereview.chromium.org/12504006/diff/4002/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (left):

https://codereview.chromium.org/12504006/diff/4002/pkg/http/test/utils.dart#o...
pkg/http/test/utils.dart:68: var encoding = requiredEncodingForCharset(
If you've removed all references to requiredEncodingForCharset, delete the
function. If you haven't, why not?

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

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:532: Encoding encoding;
This seems like it's supposed to be read-only.

It's confusing that this and charset both exist at the same time, especially
since a programmer can't rely on [encoding] being non-null. It's also weird that
currently the only way to get an encoding from its string name is to instantiate
a ContentType and read its encoding.

I'd suggest adding an Encoding.fromName constructor and removing this field.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart
File sdk/lib/io/io_sink.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:19: factory IOSink(StreamConsumer<List<int>, T> target,
Encoding encoding)
It looks like you're almost always initializing IOSinks with encoding set to
ASCII. First of all, it seems like UTF-8 would be a better default; otherwise
we're going to end up with a lot of Dart programs that seem functional when
testing but break as soon as they encounter non-ASCII text. Second, it seems
like encoding should be an optional parameter.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:22: Encoding encoding;
Either mention here or in the http documentation that HTTP-based IOSinks take
their encoding from the associated ContentType. Also explain what happens if the
content-type is unspecified, or if it is specified with an unknown encoding.

[Mads Ager (google), 2013-03-07 07:57:36]

LGTM with a couple of nits on top of Nathan's comments.

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.d...
runtime/bin/socket_patch.dart:773: 
Nit: remove the empty line or consistently have one between the write methods?

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:471: IOSink<File> openWrite([FileMode mode =
FileMode.WRITE,
Now that we have more optional arguments here I would prefer them to be named.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:1574: 
Nit: consistently have blank line between write methods or remove this one?

[Anders Johnsen, 2013-03-07 08:48:44]

LGTM

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.d...
runtime/bin/socket_patch.dart:784: void add(List<int> data) {
Remove these?

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:472: Encoding encoding = Encoding.ASCII]) {
We default to Utf8 every other place in IO. Why is this ASCII? I can understand
why on Socket, but not so much here.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart
File sdk/lib/io/io_sink.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:43: @deprecated
Can't we just remote them now? They ain't part of M3.

[Søren Gjesse, 2013-03-07 16:28:47]

PTAL

Addressed review comments

Changed addStream to writeStream on IOSink as well.

https://codereview.chromium.org/12504006/diff/4002/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (left):

https://codereview.chromium.org/12504006/diff/4002/pkg/http/test/utils.dart#o...
pkg/http/test/utils.dart:68: var encoding = requiredEncodingForCharset(
On 2013/03/06 20:31:51, nweiz wrote:
> If you've removed all references to requiredEncodingForCharset, delete the
> function. If you haven't, why not?

Done.

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.d...
runtime/bin/socket_patch.dart:773: 
On 2013/03/07 07:57:36, Mads Ager wrote:
> Nit: remove the empty line or consistently have one between the write methods?

Done.

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.d...
runtime/bin/socket_patch.dart:784: void add(List<int> data) {
On 2013/03/07 08:48:44, Anders Johnsen wrote:
> Remove these?

Done.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:471: IOSink<File> openWrite([FileMode mode =
FileMode.WRITE,
On 2013/03/07 07:57:36, Mads Ager wrote:
> Now that we have more optional arguments here I would prefer them to be named.

Done.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:472: Encoding encoding = Encoding.ASCII]) {
On 2013/03/07 08:48:44, Anders Johnsen wrote:
> We default to Utf8 every other place in IO. Why is this ASCII? I can
understand
> why on Socket, but not so much here.

Changed to UTF-8.

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

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:532: Encoding encoding;
On 2013/03/06 20:31:51, nweiz wrote:
> This seems like it's supposed to be read-only.
> 
> It's confusing that this and charset both exist at the same time, especially
> since a programmer can't rely on [encoding] being non-null. It's also weird
that
> currently the only way to get an encoding from its string name is to
instantiate
> a ContentType and read its encoding.
> 
> I'd suggest adding an Encoding.fromName constructor and removing this field.

Good point. Added static function fromName to Encoding and removed this.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:1574: 
On 2013/03/07 07:57:36, Mads Ager wrote:
> Nit: consistently have blank line between write methods or remove this one?

Done.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart
File sdk/lib/io/io_sink.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:19: factory IOSink(StreamConsumer<List<int>, T> target,
Encoding encoding)
On 2013/03/06 20:31:51, nweiz wrote:
> It looks like you're almost always initializing IOSinks with encoding set to
> ASCII. First of all, it seems like UTF-8 would be a better default; otherwise
> we're going to end up with a lot of Dart programs that seem functional when
> testing but break as soon as they encounter non-ASCII text. Second, it seems
> like encoding should be an optional parameter.

Changed encoding to be optional with a default value of Encoding.UTF_8.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:22: Encoding encoding;
On 2013/03/06 20:31:51, nweiz wrote:
> Either mention here or in the http documentation that HTTP-based IOSinks take
> their encoding from the associated ContentType. Also explain what happens if
the
> content-type is unspecified, or if it is specified with an unknown encoding.

Added documentation to IOSink, HttpClientRequest and HttpResponse.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:43: @deprecated
On 2013/03/07 08:48:44, Anders Johnsen wrote:
> Can't we just remote them now? They ain't part of M3.

Done.

[Søren Gjesse, 2013-03-07 16:28:47]

PTAL

Addressed review comments

Changed addStream to writeStream on IOSink as well.

https://codereview.chromium.org/12504006/diff/4002/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (left):

https://codereview.chromium.org/12504006/diff/4002/pkg/http/test/utils.dart#o...
pkg/http/test/utils.dart:68: var encoding = requiredEncodingForCharset(
On 2013/03/06 20:31:51, nweiz wrote:
> If you've removed all references to requiredEncodingForCharset, delete the
> function. If you haven't, why not?

Done.

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.d...
runtime/bin/socket_patch.dart:773: 
On 2013/03/07 07:57:36, Mads Ager wrote:
> Nit: remove the empty line or consistently have one between the write methods?

Done.

https://codereview.chromium.org/12504006/diff/4002/runtime/bin/socket_patch.d...
runtime/bin/socket_patch.dart:784: void add(List<int> data) {
On 2013/03/07 08:48:44, Anders Johnsen wrote:
> Remove these?

Done.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:471: IOSink<File> openWrite([FileMode mode =
FileMode.WRITE,
On 2013/03/07 07:57:36, Mads Ager wrote:
> Now that we have more optional arguments here I would prefer them to be named.

Done.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/file_impl.dart#...
sdk/lib/io/file_impl.dart:472: Encoding encoding = Encoding.ASCII]) {
On 2013/03/07 08:48:44, Anders Johnsen wrote:
> We default to Utf8 every other place in IO. Why is this ASCII? I can
understand
> why on Socket, but not so much here.

Changed to UTF-8.

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

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:532: Encoding encoding;
On 2013/03/06 20:31:51, nweiz wrote:
> This seems like it's supposed to be read-only.
> 
> It's confusing that this and charset both exist at the same time, especially
> since a programmer can't rely on [encoding] being non-null. It's also weird
that
> currently the only way to get an encoding from its string name is to
instantiate
> a ContentType and read its encoding.
> 
> I'd suggest adding an Encoding.fromName constructor and removing this field.

Good point. Added static function fromName to Encoding and removed this.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:1574: 
On 2013/03/07 07:57:36, Mads Ager wrote:
> Nit: consistently have blank line between write methods or remove this one?

Done.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart
File sdk/lib/io/io_sink.dart (right):

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:19: factory IOSink(StreamConsumer<List<int>, T> target,
Encoding encoding)
On 2013/03/06 20:31:51, nweiz wrote:
> It looks like you're almost always initializing IOSinks with encoding set to
> ASCII. First of all, it seems like UTF-8 would be a better default; otherwise
> we're going to end up with a lot of Dart programs that seem functional when
> testing but break as soon as they encounter non-ASCII text. Second, it seems
> like encoding should be an optional parameter.

Changed encoding to be optional with a default value of Encoding.UTF_8.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:22: Encoding encoding;
On 2013/03/06 20:31:51, nweiz wrote:
> Either mention here or in the http documentation that HTTP-based IOSinks take
> their encoding from the associated ContentType. Also explain what happens if
the
> content-type is unspecified, or if it is specified with an unknown encoding.

Added documentation to IOSink, HttpClientRequest and HttpResponse.

https://codereview.chromium.org/12504006/diff/4002/sdk/lib/io/io_sink.dart#ne...
sdk/lib/io/io_sink.dart:43: @deprecated
On 2013/03/07 08:48:44, Anders Johnsen wrote:
> Can't we just remote them now? They ain't part of M3.

Done.

[Anders Johnsen, 2013-03-07 16:53:49]

LGTM

https://codereview.chromium.org/12504006/diff/25001/samples/chat/chat_server_...
File samples/chat/chat_server_lib.dart (right):

https://codereview.chromium.org/12504006/diff/25001/samples/chat/chat_server_...
samples/chat/chat_server_lib.dart:314: _redirectPage =
redirectPageHtml.codeUnits;
Not your code, encodeUtf8 or remove '.codeUnits'?

https://codereview.chromium.org/12504006/diff/25001/samples/chat/chat_server_...
samples/chat/chat_server_lib.dart:320: response.write(_redirectPage);
writeBytes?

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file.dart#newc...
sdk/lib/io/file.dart:184: Encoding encoding: Encoding.UTF_8});
Thinking about this API change, I'm EXTREMELY excited about stuff like this.
This is a move in the right direction! :) Thanks Soeren!

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file_impl.dart...
sdk/lib/io/file_impl.dart:556: sink.done
Maybe do:

- 'return sink.done ...' here
- Remove completer.
- In catch statement, do 'return new Future.immediateError(e);'

That way we avoid the extra completer.

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

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:681: * header of the response. Then the header has been set
up the methods
Then -> When ?

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:683: * response. When one of the [IOSink] methods this is
used for the
Remove 'this'.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:889: * header of the request. Then the header has been set
up the methods
Then -> When

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:891: * request. When one of the [IOSink] methods this is
used for the
Remove 'this'.

[nweiz, 2013-03-07 19:32:05]

https://codereview.chromium.org/12504006/diff/25001/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (left):

https://codereview.chromium.org/12504006/diff/25001/pkg/http/lib/src/utils.da...
pkg/http/lib/src/utils.dart:88: Encoding requiredEncodingForCharset(String
charset) {
Now I'm concerned that there are cases where [requiredEncodingFromCharset] was
being used that no longer throw FormatExceptions. Perhaps the right thing to do
is to leave it in after all and just implement it in terms of Encoding.fromName.

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

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:703: * If an unsupported encoding is used an exception will
be thrown.
Specify that the exception will only be thrown when [IOSink.write] is called.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:911: * If an unsupported encoding is used an exception will
be thrown.
Also here.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/string_transfo...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/string_transfo...
sdk/lib/io/string_transformer.dart:36: }
It would be nice if we could support all the character set aliases listed in
http://www.iana.org/assignments/character-sets/character-sets.xml.

[Søren Gjesse, 2013-03-08 09:47:46]

https://codereview.chromium.org/12504006/diff/25001/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (left):

https://codereview.chromium.org/12504006/diff/25001/pkg/http/lib/src/utils.da...
pkg/http/lib/src/utils.dart:88: Encoding requiredEncodingForCharset(String
charset) {
On 2013/03/07 19:32:05, nweiz wrote:
> Now I'm concerned that there are cases where [requiredEncodingFromCharset] was
> being used that no longer throw FormatExceptions. Perhaps the right thing to
do
> is to leave it in after all and just implement it in terms of
Encoding.fromName.

Done.

https://codereview.chromium.org/12504006/diff/25001/samples/chat/chat_server_...
File samples/chat/chat_server_lib.dart (right):

https://codereview.chromium.org/12504006/diff/25001/samples/chat/chat_server_...
samples/chat/chat_server_lib.dart:314: _redirectPage =
redirectPageHtml.codeUnits;
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Not your code, encodeUtf8 or remove '.codeUnits'?

This builds on the assumption that the redirectPageHtml is all Latin-1
characters, which should be fine.

https://codereview.chromium.org/12504006/diff/25001/samples/chat/chat_server_...
samples/chat/chat_server_lib.dart:320: response.write(_redirectPage);
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> writeBytes?

Absolutely, thanks.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file.dart#newc...
sdk/lib/io/file.dart:184: Encoding encoding: Encoding.UTF_8});
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Thinking about this API change, I'm EXTREMELY excited about stuff like this.
> This is a move in the right direction! :) Thanks Soeren!

Thanks.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/file_impl.dart...
sdk/lib/io/file_impl.dart:556: sink.done
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Maybe do:
> 
> - 'return sink.done ...' here
> - Remove completer.
> - In catch statement, do 'return new Future.immediateError(e);'
> 
> That way we avoid the extra completer.

Thanks, that is mich nicer. Had to do

  return sink.done.then((_) => this);;

for the future to complete with the File object instead of the IOSink.

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

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:681: * header of the response. Then the header has been set
up the methods
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Then -> When ?

Done.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:683: * response. When one of the [IOSink] methods this is
used for the
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Remove 'this'.

Done.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:703: * If an unsupported encoding is used an exception will
be thrown.
On 2013/03/07 19:32:05, nweiz wrote:
> Specify that the exception will only be thrown when [IOSink.write] is called.

Done.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:889: * header of the request. Then the header has been set
up the methods
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Then -> When

Done.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:891: * request. When one of the [IOSink] methods this is
used for the
On 2013/03/07 16:53:49, Anders Johnsen wrote:
> Remove 'this'.

Done.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/http.dart#newc...
sdk/lib/io/http.dart:911: * If an unsupported encoding is used an exception will
be thrown.
On 2013/03/07 19:32:05, nweiz wrote:
> Also here.

Done.

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/string_transfo...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/12504006/diff/25001/sdk/lib/io/string_transfo...
sdk/lib/io/string_transformer.dart:36: }
On 2013/03/07 19:32:05, nweiz wrote:
> It would be nice if we could support all the character set aliases listed in
> http://www.iana.org/assignments/character-sets/character-sets.xml.

Done.

[Søren Gjesse, 2013-03-08 10:07:05]

Committed patchset #7 manually as r19676 (presubmit successful).

[nweiz, 2013-03-08 19:37:20]

lgtm