Adding streams to dart:html.

Adding streams to dart:html.

This exposes all DOM events as streams. This keeps the current DOM event syntax as well, I'd like to make sure this API is stable before deprecating the old syntax, then we'll remove it a while after that.

I made an initial stab at getting the event types correct for the streams, though it's not documented very well and frequently inconsistent between browsers. Hopefully it's close, and I imagine there will be changes to come.


BUG=

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

[blois, 2013-01-10 22:10:38]

Here's an initial implementation of exposing DOM events as streams.

The goal here is that DOM events should be exposed the same way that application
model events are exposed. I'm still a bit concerned that people are not
considering model events as streams, but hopefully this change raises awareness
and helps flush out any issues if there are any.

[floitsch, 2013-01-11 09:44:06]

Thanks.
What do you think of renaming the streams to plural and removing the "on" ?
for example: foo.clicks.listen(...)

[Jennifer Messerly, 2013-01-11 21:57:17]

It would be good to try and measure the additional memory overhead of each
event.

Maybe it's swamped by the overhead of the DOM addEventListener, or maybe not.
Not sure. It certainly seems like we're allocating a lot of objects compared to
the previous approach (one StreamController and its associated data per event
hookup -- if I'm understanding correctly), moreover, those objects are live and
so will not be collected until the event is unregistered.

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
File tests/html/streams_test.dart (right):

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
their events.
Q: what is the effect of unsubscribeOnError? I think it means the chain is
broken, in which case might be worth adding test for that

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
tests/html/streams_test.dart:38: }, unsubscribeOnError:true);
I think we usually have space after : in named args:

http://www.dartlang.org/articles/style-guide/#do-use-a-space-around--in-named...

https://codereview.chromium.org/11824072/diff/1/tools/dom/scripts/htmleventge...
File tools/dom/scripts/htmleventgenerator.py (right):

https://codereview.chromium.org/11824072/diff/1/tools/dom/scripts/htmleventge...
tools/dom/scripts/htmleventgenerator.py:411: "  Stream<$TYPE> get $(NAME) =>
$PROVIDER.forTarget(this);\n",
one concern here:

every time someone registers an event, it now creates a new StreamController,
correct?

1. is it a concern that the values returned by sequentially calling the getter
doesn't respect identity or equality? even though conceptually nothing has
changed.

2. do we have an idea of the memory overhead this causes compared to
$dom_addEventListener or the previous .on approach?

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
File tools/dom/src/HtmlStreamController.dart (right):

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
extends Event> extends StreamController<T> {
High level question: do we want to subclass StreamController?
a problem with StreamController is that it implements StreamSink, meaning "add"
and "close" are visible. That is the meaning of calling these?

I worry about "add" because, since these StreamControllers aren't cached for a
particular element, someone might call "add" and expect to be dispatching an
event. But it won't actually work.

Another thing I noticed, StreamController has an extra level of indirection. I
wonder if it would be better to inherit from Stream directly. You could
potentially avoid some bookeeping, at the cost of extra implementation work.

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
tools/dom/src/HtmlStreamController.dart:18: _eventHandler = _handleEvent;
Can this just be:

    _eventHandler = this.add;

in other words, do you need _handleEvent?

[Jennifer Messerly, 2013-01-11 21:59:06]

by the way -- I do like the APIs for this. Especially the typed event args, and
that you can now add named events to other elements in the DOM using .forTarget.

On 2013/01/11 21:57:17, John Messerly wrote:
> It would be good to try and measure the additional memory overhead of each
> event.
> 
> Maybe it's swamped by the overhead of the DOM addEventListener, or maybe not.
> Not sure. It certainly seems like we're allocating a lot of objects compared
to
> the previous approach (one StreamController and its associated data per event
> hookup -- if I'm understanding correctly), moreover, those objects are live
and
> so will not be collected until the event is unregistered.
> 
> https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
> File tests/html/streams_test.dart (right):
> 
>
https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
> tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
> their events.
> Q: what is the effect of unsubscribeOnError? I think it means the chain is
> broken, in which case might be worth adding test for that
> 
>
https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
> tests/html/streams_test.dart:38: }, unsubscribeOnError:true);
> I think we usually have space after : in named args:
> 
>
http://www.dartlang.org/articles/style-guide/#do-use-a-space-around--in-named...
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/scripts/htmleventge...
> File tools/dom/scripts/htmleventgenerator.py (right):
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/scripts/htmleventge...
> tools/dom/scripts/htmleventgenerator.py:411: "  Stream<$TYPE> get $(NAME) =>
> $PROVIDER.forTarget(this);\n",
> one concern here:
> 
> every time someone registers an event, it now creates a new StreamController,
> correct?
> 
> 1. is it a concern that the values returned by sequentially calling the getter
> doesn't respect identity or equality? even though conceptually nothing has
> changed.
> 
> 2. do we have an idea of the memory overhead this causes compared to
> $dom_addEventListener or the previous .on approach?
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> File tools/dom/src/HtmlStreamController.dart (right):
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
> extends Event> extends StreamController<T> {
> High level question: do we want to subclass StreamController?
> a problem with StreamController is that it implements StreamSink, meaning
"add"
> and "close" are visible. That is the meaning of calling these?
> 
> I worry about "add" because, since these StreamControllers aren't cached for a
> particular element, someone might call "add" and expect to be dispatching an
> event. But it won't actually work.
> 
> Another thing I noticed, StreamController has an extra level of indirection. I
> wonder if it would be better to inherit from Stream directly. You could
> potentially avoid some bookeeping, at the cost of extra implementation work.
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> tools/dom/src/HtmlStreamController.dart:18: _eventHandler = _handleEvent;
> Can this just be:
> 
>     _eventHandler = this.add;
> 
> in other words, do you need _handleEvent?

[floitsch, 2013-01-11 22:46:11]

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
File tools/dom/src/HtmlStreamController.dart (right):

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
extends Event> extends StreamController<T> {
On 2013/01/11 21:57:17, John Messerly wrote:
> High level question: do we want to subclass StreamController?
> a problem with StreamController is that it implements StreamSink, meaning
"add"
> and "close" are visible. That is the meaning of calling these?
> 
> I worry about "add" because, since these StreamControllers aren't cached for a
> particular element, someone might call "add" and expect to be dispatching an
> event. But it won't actually work.
> 
> Another thing I noticed, StreamController has an extra level of indirection. I
> wonder if it would be better to inherit from Stream directly. You could
> potentially avoid some bookeeping, at the cost of extra implementation work.

If I'm not wrong we can directly forward the registration request to the DOM,
and just return a subscription object that knows how to remove the event
listener.
Normally there shouldn't be a need for a list of subscribers.

If there is something that makes this impossible, we should see if we can fix
it.

[blois, 2013-01-11 22:51:30]

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
File tests/html/streams_test.dart (right):

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
their events.
On 2013/01/11 21:57:17, John Messerly wrote:
> Q: what is the effect of unsubscribeOnError? I think it means the chain is
> broken, in which case might be worth adding test for that

We have no way of triggering unsubscribeOnError for DOM events- it would have to
come from a subsequent stream. From what I understand, unsubscribeOnError is
essentially a no-op for DOM event streams.

Not sure how to further test this without just re-iterating core streams tests,
but I can definitely add a test if I'm missing an angle.

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
tests/html/streams_test.dart:38: }, unsubscribeOnError:true);
On 2013/01/11 21:57:17, John Messerly wrote:
> I think we usually have space after : in named args:
> 
>
http://www.dartlang.org/articles/style-guide/#do-use-a-space-around--in-named...

Done.

https://codereview.chromium.org/11824072/diff/1/tools/dom/scripts/htmleventge...
File tools/dom/scripts/htmleventgenerator.py (right):

https://codereview.chromium.org/11824072/diff/1/tools/dom/scripts/htmleventge...
tools/dom/scripts/htmleventgenerator.py:411: "  Stream<$TYPE> get $(NAME) =>
$PROVIDER.forTarget(this);\n",
On 2013/01/11 21:57:17, John Messerly wrote:
> one concern here:
> 
> every time someone registers an event, it now creates a new StreamController,
> correct?
> 
> 1. is it a concern that the values returned by sequentially calling the getter
> doesn't respect identity or equality? even though conceptually nothing has
> changed.
> 
> 2. do we have an idea of the memory overhead this causes compared to
> $dom_addEventListener or the previous .on approach?

1. I believe that the Stream API is essentially immutable, so returning a new
instance each time should be OK. My expectation is that the vast majority of
events are listened to by a single listener and that a per-instance sparse cache
would create overhead with very little benefit.

2. I did some basic runtime performance tests a while back but can do some more,
but would be great if someone from the Library team could quantify performance
differences between Streams and a lightweight Event class.

I think the performance aspect is even more important when using Streams for
model events.

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
File tools/dom/src/HtmlStreamController.dart (right):

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
extends Event> extends StreamController<T> {
On 2013/01/11 22:46:11, floitsch wrote:
> On 2013/01/11 21:57:17, John Messerly wrote:
> > High level question: do we want to subclass StreamController?
> > a problem with StreamController is that it implements StreamSink, meaning
> "add"
> > and "close" are visible. That is the meaning of calling these?
> > 
> > I worry about "add" because, since these StreamControllers aren't cached for
a
> > particular element, someone might call "add" and expect to be dispatching an
> > event. But it won't actually work.
> > 
> > Another thing I noticed, StreamController has an extra level of indirection.
I
> > wonder if it would be better to inherit from Stream directly. You could
> > potentially avoid some bookeeping, at the cost of extra implementation work.
> 
> If I'm not wrong we can directly forward the registration request to the DOM,
> and just return a subscription object that knows how to remove the event
> listener.
> Normally there shouldn't be a need for a list of subscribers.
> 
> If there is something that makes this impossible, we should see if we can fix
> it.

Will take a look at this now.

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
tools/dom/src/HtmlStreamController.dart:18: _eventHandler = _handleEvent;
On 2013/01/11 21:57:17, John Messerly wrote:
> Can this just be:
> 
>     _eventHandler = this.add;
> 
> in other words, do you need _handleEvent?

Unfortunately, this fails in dart2js checked mode.
http://code.google.com/p/dart/issues/detail?id=7754

Added a comment to this extent.

[floitsch, 2013-01-11 22:55:45]

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
File tests/html/streams_test.dart (right):

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
their events.
On 2013/01/11 22:51:30, blois wrote:
> On 2013/01/11 21:57:17, John Messerly wrote:
> > Q: what is the effect of unsubscribeOnError? I think it means the chain is
> > broken, in which case might be worth adding test for that
> 
> We have no way of triggering unsubscribeOnError for DOM events- it would have
to
> come from a subsequent stream. From what I understand, unsubscribeOnError is
> essentially a no-op for DOM event streams.
> 
> Not sure how to further test this without just re-iterating core streams
tests,
> but I can definitely add a test if I'm missing an angle.

Correct. unsubscribeOnError (similar to the onError and onDone handlers) is not
used for the Html Streams.

[Jennifer Messerly, 2013-01-11 23:30:14]

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
File tests/html/streams_test.dart (right):

https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
their events.
On 2013/01/11 22:55:45, floitsch wrote:
> On 2013/01/11 22:51:30, blois wrote:
> > On 2013/01/11 21:57:17, John Messerly wrote:
> > > Q: what is the effect of unsubscribeOnError? I think it means the chain is
> > > broken, in which case might be worth adding test for that
> > 
> > We have no way of triggering unsubscribeOnError for DOM events- it would
have
> to
> > come from a subsequent stream. From what I understand, unsubscribeOnError is
> > essentially a no-op for DOM event streams.
> > 
> > Not sure how to further test this without just re-iterating core streams
> tests,
> > but I can definitely add a test if I'm missing an angle.
> 
> Correct. unsubscribeOnError (similar to the onError and onDone handlers) is
not
> used for the Html Streams.

Makes sense. Thanks for clarifying!

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
File tools/dom/src/HtmlStreamController.dart (right):

https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
extends Event> extends StreamController<T> {
On 2013/01/11 22:51:30, blois wrote:
> On 2013/01/11 22:46:11, floitsch wrote:
> > On 2013/01/11 21:57:17, John Messerly wrote:
> > > High level question: do we want to subclass StreamController?
> > > a problem with StreamController is that it implements StreamSink, meaning
> > "add"
> > > and "close" are visible. That is the meaning of calling these?
> > > 
> > > I worry about "add" because, since these StreamControllers aren't cached
for
> a
> > > particular element, someone might call "add" and expect to be dispatching
an
> > > event. But it won't actually work.
> > > 
> > > Another thing I noticed, StreamController has an extra level of
indirection.
> I
> > > wonder if it would be better to inherit from Stream directly. You could
> > > potentially avoid some bookeeping, at the cost of extra implementation
work.
> > 
> > If I'm not wrong we can directly forward the registration request to the
DOM,
> > and just return a subscription object that knows how to remove the event
> > listener.
> > Normally there shouldn't be a need for a list of subscribers.
> > 
> > If there is something that makes this impossible, we should see if we can
fix
> > it.
> 
> Will take a look at this now.

I hope it works -- that would be awesome :)

[blois, 2013-01-12 00:41:46]

On 2013/01/11 23:30:14, John Messerly wrote:
> https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
> File tests/html/streams_test.dart (right):
> 
>
https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
> tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
> their events.
> On 2013/01/11 22:55:45, floitsch wrote:
> > On 2013/01/11 22:51:30, blois wrote:
> > > On 2013/01/11 21:57:17, John Messerly wrote:
> > > > Q: what is the effect of unsubscribeOnError? I think it means the chain
is
> > > > broken, in which case might be worth adding test for that
> > > 
> > > We have no way of triggering unsubscribeOnError for DOM events- it would
> have
> > to
> > > come from a subsequent stream. From what I understand, unsubscribeOnError
is
> > > essentially a no-op for DOM event streams.
> > > 
> > > Not sure how to further test this without just re-iterating core streams
> > tests,
> > > but I can definitely add a test if I'm missing an angle.
> > 
> > Correct. unsubscribeOnError (similar to the onError and onDone handlers) is
> not
> > used for the Html Streams.
> 
> Makes sense. Thanks for clarifying!
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> File tools/dom/src/HtmlStreamController.dart (right):
> 
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
> extends Event> extends StreamController<T> {
> On 2013/01/11 22:51:30, blois wrote:
> > On 2013/01/11 22:46:11, floitsch wrote:
> > > On 2013/01/11 21:57:17, John Messerly wrote:
> > > > High level question: do we want to subclass StreamController?
> > > > a problem with StreamController is that it implements StreamSink,
meaning
> > > "add"
> > > > and "close" are visible. That is the meaning of calling these?
> > > > 
> > > > I worry about "add" because, since these StreamControllers aren't cached
> for
> > a
> > > > particular element, someone might call "add" and expect to be
dispatching
> an
> > > > event. But it won't actually work.
> > > > 
> > > > Another thing I noticed, StreamController has an extra level of
> indirection.
> > I
> > > > wonder if it would be better to inherit from Stream directly. You could
> > > > potentially avoid some bookeeping, at the cost of extra implementation
> work.
> > > 
> > > If I'm not wrong we can directly forward the registration request to the
> DOM,
> > > and just return a subscription object that knows how to remove the event
> > > listener.
> > > Normally there shouldn't be a need for a list of subscribers.
> > > 
> > > If there is something that makes this impossible, we should see if we can
> fix
> > > it.
> > 
> > Will take a look at this now.
> 
> I hope it works -- that would be awesome :)


Updated to subclass Stream/StreamSubscription instead- I believe that this has
the appropriate behavior but would greatly appreciate a second look.

I do think this is a much improved flow.

One thing I'm missing is the asMultiSubscriberStream API- I'm not quite sure how
this should work, suggestions?

Note that I also renamed HtmlStreamProvider to EventStreamProvider since these
are technically DOM events rather than HTML events (but DomEventStreamProvider
is too cumbersome).

[floitsch, 2013-01-12 00:44:18]

On 2013/01/12 00:41:46, blois wrote:
> On 2013/01/11 23:30:14, John Messerly wrote:
> > https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart
> > File tests/html/streams_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/11824072/diff/1/tests/html/streams_test.dart#...
> > tests/html/streams_test.dart:34: // Unsubscribe on error handlers still get
> > their events.
> > On 2013/01/11 22:55:45, floitsch wrote:
> > > On 2013/01/11 22:51:30, blois wrote:
> > > > On 2013/01/11 21:57:17, John Messerly wrote:
> > > > > Q: what is the effect of unsubscribeOnError? I think it means the
chain
> is
> > > > > broken, in which case might be worth adding test for that
> > > > 
> > > > We have no way of triggering unsubscribeOnError for DOM events- it would
> > have
> > > to
> > > > come from a subsequent stream. From what I understand,
unsubscribeOnError
> is
> > > > essentially a no-op for DOM event streams.
> > > > 
> > > > Not sure how to further test this without just re-iterating core streams
> > > tests,
> > > > but I can definitely add a test if I'm missing an angle.
> > > 
> > > Correct. unsubscribeOnError (similar to the onError and onDone handlers)
is
> > not
> > > used for the Html Streams.
> > 
> > Makes sense. Thanks for clarifying!
> > 
> >
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> > File tools/dom/src/HtmlStreamController.dart (right):
> > 
> >
>
https://codereview.chromium.org/11824072/diff/1/tools/dom/src/HtmlStreamContr...
> > tools/dom/src/HtmlStreamController.dart:10: class _HtmlStreamController<T
> > extends Event> extends StreamController<T> {
> > On 2013/01/11 22:51:30, blois wrote:
> > > On 2013/01/11 22:46:11, floitsch wrote:
> > > > On 2013/01/11 21:57:17, John Messerly wrote:
> > > > > High level question: do we want to subclass StreamController?
> > > > > a problem with StreamController is that it implements StreamSink,
> meaning
> > > > "add"
> > > > > and "close" are visible. That is the meaning of calling these?
> > > > > 
> > > > > I worry about "add" because, since these StreamControllers aren't
cached
> > for
> > > a
> > > > > particular element, someone might call "add" and expect to be
> dispatching
> > an
> > > > > event. But it won't actually work.
> > > > > 
> > > > > Another thing I noticed, StreamController has an extra level of
> > indirection.
> > > I
> > > > > wonder if it would be better to inherit from Stream directly. You
could
> > > > > potentially avoid some bookeeping, at the cost of extra implementation
> > work.
> > > > 
> > > > If I'm not wrong we can directly forward the registration request to the
> > DOM,
> > > > and just return a subscription object that knows how to remove the event
> > > > listener.
> > > > Normally there shouldn't be a need for a list of subscribers.
> > > > 
> > > > If there is something that makes this impossible, we should see if we
can
> > fix
> > > > it.
> > > 
> > > Will take a look at this now.
> > 
> > I hope it works -- that would be awesome :)
> 
> 
> Updated to subclass Stream/StreamSubscription instead- I believe that this has
> the appropriate behavior but would greatly appreciate a second look.
> 
> I do think this is a much improved flow.
> 
> One thing I'm missing is the asMultiSubscriberStream API- I'm not quite sure
how
> this should work, suggestions?
DOM streams are automatically multi-subscribers. So just returning "this" is the
correct behavior.

> 
> Note that I also renamed HtmlStreamProvider to EventStreamProvider since these
> are technically DOM events rather than HTML events (but DomEventStreamProvider
> is too cumbersome).

[floitsch, 2013-01-12 00:49:33]

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:23: throw new ArgumentError('onError is
not supported by DOM events');
I would just discard it. It is supported, but the stream will just never emit
it.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:25: if (onDone != null) {
ditto.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:28: if (unsubscribeOnError != null) {
ditto.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:38: class _EventStreamSubscription<T
extends Event> extends StreamSubscription<T> {
This is pretty much what I was looking for.

I don't remember all contracts by heart (so I'm not sure if you are allowed to
resume a non-paused stream...), but otherwise LGTM!

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:74: throw new UnsupportedError('Not
supported by DOM events');
ditto: just ignore it.

[Jennifer Messerly, 2013-01-12 01:11:25]

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:38: class _EventStreamSubscription<T
extends Event> extends StreamSubscription<T> {
On 2013/01/12 00:49:33, floitsch wrote:
> This is pretty much what I was looking for.
> 
> I don't remember all contracts by heart (so I'm not sure if you are allowed to
> resume a non-paused stream...), but otherwise LGTM!

+1, this is so nice

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:56: // Clear out the target to indicate
this is complete.
also has the nice side effect of releasing memory :)
might want to set _onData to null too, either here or in _unlisten

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:69: _onData = handleData;
is this allowed to be null? I see some checks for it, but wasn't sure how it can
happen

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:93: void resume() {
this should probably check if paused. The _StreamSubscriptionImpl does this:

    if (!isPaused) {
      throw new StateError("Resuming unpaused subscription");
    }

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:112:
_target.$dom_removeEventListener(_eventType, _onData, _useCapture);
should this set _onData to null after unregistering?

[Jennifer Messerly, 2013-01-12 01:13:00]

One more question -- how is "dispatch" handled in this model?

(We used to have it on EventListenerList)

[blois, 2013-01-12 01:42:15]

On 2013/01/12 01:13:00, John Messerly wrote:
> One more question -- how is "dispatch" handled in this model?
> 
> (We used to have it on EventListenerList)

Could expose either EventTarget.dispatch(Event) or
EventStreamProvider.dispatch(Event). I believe that Jacob had a preference for
EventTarget.dispatch.

[blois, 2013-01-12 01:42:52]

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:23: throw new ArgumentError('onError is
not supported by DOM events');
On 2013/01/12 00:49:33, floitsch wrote:
> I would just discard it. It is supported, but the stream will just never emit
> it.

Done.

Old compatibility habits- always throw exceptions in case support is added
later, otherwise it'd be a breaking change (.NET rules at least!)

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:25: if (onDone != null) {
On 2013/01/12 00:49:33, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:28: if (unsubscribeOnError != null) {
On 2013/01/12 00:49:33, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:56: // Clear out the target to indicate
this is complete.
On 2013/01/12 01:11:25, John Messerly wrote:
> also has the nice side effect of releasing memory :)
> might want to set _onData to null too, either here or in _unlisten

Done.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:69: _onData = handleData;
On 2013/01/12 01:11:25, John Messerly wrote:
> is this allowed to be null? I see some checks for it, but wasn't sure how it
can
> happen

onData can be null from the listen call and I believe it can be set to null at
any point. I added a test for these cases.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:74: throw new UnsupportedError('Not
supported by DOM events');
On 2013/01/12 00:49:33, floitsch wrote:
> ditto: just ignore it.

Done.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:93: void resume() {
On 2013/01/12 01:11:25, John Messerly wrote:
> this should probably check if paused. The _StreamSubscriptionImpl does this:
> 
>     if (!isPaused) {
>       throw new StateError("Resuming unpaused subscription");
>     }

Done.

https://codereview.chromium.org/11824072/diff/12001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:112:
_target.$dom_removeEventListener(_eventType, _onData, _useCapture);
On 2013/01/12 01:11:25, John Messerly wrote:
> should this set _onData to null after unregistering?
> 
We unlisten when it's paused, but can resume later. I added clearing _onData in
cancel since that's not recoverable.

[Jennifer Messerly, 2013-01-12 01:47:00]

lgtm