Make MultiChannel follow the stream channel rules.

Make MultiChannel follow the stream channel rules.

It was written before the rules were fully fleshed-out, and I forgot to
update it to comply.

R=tjblasi@google.com

Committed: https://github.com/dart-lang/stream_channel/commit/0842485ee2be1c62e276d0e988aedc525bf29181

[tjblasi, 2016-02-10 22:10:10]

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart
File lib/src/multi_channel.dart (right):

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
lib/src/multi_channel.dart:147: onDone: _closeInnerChannel,
This looks like odd indentation to me, but if dartfmt says it's good then it's
good enough for me.

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
lib/src/multi_channel.dart:156: id = _nextId + 1;
This is well explained below & in the comments above `_nextId`, but here this is
rather opaque.

Could the id generation be encapsulated in its own methods or an object of its
own? Something like `nextLocalId` and `nextRemoteId` with docs similar to those
elsewhere in this file could make this easier to grok.

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
lib/src/multi_channel.dart:217: for (var controller in
_controllers.values.toList()) {
nit: I think `new List.from(_controller.values)` would aid readability.

If you think it might someday matter, could add `growable: false` also.

[nweiz, 2016-02-10 22:26:13]

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart
File lib/src/multi_channel.dart (right):

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
lib/src/multi_channel.dart:147: onDone: _closeInnerChannel,
On 2016/02/10 22:10:10, tjblasi wrote:
> This looks like odd indentation to me, but if dartfmt says it's good then it's
> good enough for me.

I don't generally use dartfmt, but it does happen to agree with this formatting.

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
lib/src/multi_channel.dart:156: id = _nextId + 1;
On 2016/02/10 22:10:10, tjblasi wrote:
> This is well explained below & in the comments above `_nextId`, but here this
is
> rather opaque.
> 
> Could the id generation be encapsulated in its own methods or an object of its
> own? Something like `nextLocalId` and `nextRemoteId` with docs similar to
those
> elsewhere in this file could make this easier to grok.

The trouble is, most of the time we need to return two values and Dart isn't
very good at that I can move the logic above this though, and re-use inputId.

https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
lib/src/multi_channel.dart:217: for (var controller in
_controllers.values.toList()) {
On 2016/02/10 22:10:10, tjblasi wrote:
> nit: I think `new List.from(_controller.values)` would aid readability.
> 
> If you think it might someday matter, could add `growable: false` also.

Done, although I'm curious what about this is more readable to you.

[nweiz, 2016-02-10 22:26:17]

Code review changes

[nweiz, 2016-02-10 22:26:28]

Description was changed from

==========
Make MultiChannel follow the stream channel rules.

It was written before the rules were fully fleshed-out, and I forgot to
update it to comply.

R=tjblasi@google.com
==========

to

==========
Make MultiChannel follow the stream channel rules.

It was written before the rules were fully fleshed-out, and I forgot to
update it to comply.

R=tjblasi@google.com

Committed:
https://github.com/dart-lang/stream_channel/commit/0842485ee2be1c62e276d0e988...
==========

[nweiz, 2016-02-10 22:26:28]

Committed patchset #2 (id:20001) manually as
0842485ee2be1c62e276d0e988aedc525bf29181 (presubmit successful).

[tjblasi, 2016-02-10 22:31:43]

On 2016/02/10 22:26:13, nweiz wrote:
> https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart
> File lib/src/multi_channel.dart (right):
> 
>
https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
> lib/src/multi_channel.dart:147: onDone: _closeInnerChannel,
> On 2016/02/10 22:10:10, tjblasi wrote:
> > This looks like odd indentation to me, but if dartfmt says it's good then
it's
> > good enough for me.
> 
> I don't generally use dartfmt, but it does happen to agree with this
formatting.
> 
>
https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
> lib/src/multi_channel.dart:156: id = _nextId + 1;
> On 2016/02/10 22:10:10, tjblasi wrote:
> > This is well explained below & in the comments above `_nextId`, but here
this
> is
> > rather opaque.
> > 
> > Could the id generation be encapsulated in its own methods or an object of
its
> > own? Something like `nextLocalId` and `nextRemoteId` with docs similar to
> those
> > elsewhere in this file could make this easier to grok.
> 
> The trouble is, most of the time we need to return two values and Dart isn't
> very good at that I can move the logic above this though, and re-use inputId.
> 
>
https://codereview.chromium.org/1686263002/diff/1/lib/src/multi_channel.dart#...
> lib/src/multi_channel.dart:217: for (var controller in
> _controllers.values.toList()) {
> On 2016/02/10 22:10:10, tjblasi wrote:
> > nit: I think `new List.from(_controller.values)` would aid readability.
> > 
> > If you think it might someday matter, could add `growable: false` also.
> 
> Done, although I'm curious what about this is more readable to you.

Methods named like `toList()` return a new object by convention, but `new
List.from` is very explicit about creating a new object. I think this makes it
more clear how you're avoiding the concurrent modification issue.