Making ScriptProcessorNode not an EventTarget.

tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate

+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+part of $LIBRARY;
+
+$(ANNOTATIONS)class $CLASSNAME$EXTENDS$NATIVESPEC {
+  Stream<AudioProcessingEvent> _eventStream;

[Anton Muhin, 2013/03/14 08:27:39]

please, no: we agreed that we're going to implement simple getter/setter for
now, let's stick with that, please.

[Anton Muhin, 2013/03/14 09:30:52]

My bad, please, disregard.

[Emily Fortuna, 2013/03/14 18:11:50]

I made this change because Pavel asked for it again after we'd decided the other
way. If you prefer the other way, please look at the previous patch set. It did
it the way we originally discussed.

I'm willing to do it whatever way (both implementations are saved in git); I'm
just trying to get this in so Graham can use it.

+
+  /**
+   * Get a Stream that fires events when AudioProcessingEvents occur.
+   * This particular stream is special in that it only allows one listener to a
+   * given stream. Subsequent calls to [listen] after the first call will throw
+   */
+  Stream<AudioProcessingEvent> get onAudioProcess {
+    if (_eventStream == null) {
+      _eventStream = new _AudioEventStream(this);
+    }
+    return _eventStream;
+  }
+$!MEMBERS
+}
+
+class _AudioEventStreamSubscription<AudioProcessingEvent> extends

[Anton Muhin, 2013/03/14 09:30:52]

why this generic argument?  looks like a typo.

[Emily Fortuna, 2013/03/14 18:11:50]

removed. it was left over from the general EventStreamSubscription and I had
neglected to remove it.

+    StreamSubscription<AudioProcessingEvent> {
+  EventListener _callback;

[Anton Muhin, 2013/03/14 09:30:52]

if we prohibit null _callback, is it true _callback == null iff !_paused?

[Emily Fortuna, 2013/03/14 18:11:50]

True. Fixed up. I had the paused variable to distinguish error messages in the
resume() function, but it's probably not that useful.

+  ScriptProcessorNode _target;
+  bool _paused;
+
+  _AudioEventStreamSubscription(this._target, this._callback) {
+    _paused = false;

[Anton Muhin, 2013/03/14 09:30:52]

maybe move into field declaration?

[Emily Fortuna, 2013/03/14 18:11:50]

Done.

+    _tryResume();
+  }
+
+  void cancel() {

[Anton Muhin, 2013/03/14 09:30:52]

shouldn't it also clear _AudioEventStream._streamSubscription?

[Anton Muhin, 2013/03/14 09:32:22]

cannot we have:

void cancel() { pause(); } ?

[Emily Fortuna, 2013/03/14 18:11:50]

Done.

[Emily Fortuna, 2013/03/14 18:11:50]

No, a StreamSubscription can only be associated with one Stream (which is
associated with one ScriptProcessorNode). The callback may be set to null, but
someone else that wants to "subscribe" will set this same subscription to a
different callback.

+    if (_callback != null) {
+      throw new StateError("Subscription has already been canceled.");
+    }
+    _unlisten();
+  }
+
+  void onData(void callback(AudioProcessingEvent)) {
+    if (_callback != null) {
+      throw new StateError("Subscription has been canceled.");
+    }
+    _setCallback(callback);
+  }
+
+  /// Has no effect.
+  void onError(void handleError(AsyncError error)) {}
+
+  /// Has no effect.
+  void onDone(void handleDone()) {}
+
+  void pause([Future resumeSignal]) {
+    if (_callback != null) {
+      throw new StateError("Subscription has been canceled.");
+    }
+    _unlisten();
+    _paused = true;
+
+    if (resumeSignal != null) {
+      resumeSignal.whenComplete(() { _paused = false; resume(); });
+    }
+  }
+
+  void resume() {
+    if (_callback != null) {

[Anton Muhin, 2013/03/14 09:30:52]

if (_callback == null) ?  and has been resumed?

[Emily Fortuna, 2013/03/14 18:11:50]

Done.

+      throw new StateError("Subscription has been canceled.");
+    }
+    if (!_paused) {
+      throw new StateError("Subscription is not paused.");
+    }
+    _tryResume();
+  }
+
+  void _tryResume() {
+    if (_callback != null) {
+      _setCallback(_callback);
+      _callback = null;
+    }
+  }
+
+  void _unlisten() {
+    if (_callback == null) {
+      var noop = (event) {};

[Anton Muhin, 2013/03/14 09:30:52]

why not null?

[Emily Fortuna, 2013/03/14 18:11:50]

Done.

+$if DART2JS

[Anton Muhin, 2013/03/14 09:30:52]

Why have conditional code here?  Why not have _onAudioProcess in
ScriptProcessorNode itself (with backend-specific implementation) and
backend-independent implementation here?

[Emily Fortuna, 2013/03/14 18:11:50]

Because in this version we're returning a custom Stream. If you'd like a pure
getter/setter, I believe you're looking for this imlementation:
https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...

+      _callback = JS('Function', '#.onaudioprocess', _target);
+$else
+      // TODO(podivilov): Implement on Dartium.
+$endif
+      _setCallback(noop);
+    }
+  }
+
+  void _setCallback(callbackFunc) {
+$if DART2JS
+    JS('void', '#.onaudioprocess = #', _target,
+        convertDartClosureToJS(_callback, 1));
+$else
+    // TODO(podivilov): Implement on Dartium.
+$endif
+  }
+}
+
+/**
+ * Adapter for exposing DOM events as Dart streams.
+ */
+class _AudioEventStream<AudioProcessingEvent> 

[Anton Muhin, 2013/03/14 09:30:52]

why generic argument here?

[Emily Fortuna, 2013/03/14 18:11:50]

removed

+    extends Stream<AudioProcessingEvent> {
+  static Set _createdTargets = new Set();

[Anton Muhin, 2013/03/14 09:30:52]

nit: do not we name static _CREATED_TARGETS?  And why omit types?

[Emily Fortuna, 2013/03/14 18:11:50]

I thought all caps were for constants. This isn't a constant. Generic type
added.

+  ScriptProcessorNode _target;
+  StreamSubscription _streamSubscription;
+
+  _AudioEventStream(target) {
+    if (_createdTargets.contains(target)) {

[Anton Muhin, 2013/03/14 09:30:52]

why we need this check?

Note as well, that you never remove items from this set which leads to memory
leaks.

[Emily Fortuna, 2013/03/14 18:11:50]

This was to enforce the "one listener per ScriptProcessorNode" but we can remove
it since there's only a getter that initializes this once on the
ScriptProcessorNode itself now. 

+      throw new ArgumentError('This ScriptProcessorNode already has an event '

[Anton Muhin, 2013/03/14 09:30:52]

nit: there is a mix of " and ' for string literals.

[Emily Fortuna, 2013/03/14 18:11:50]

Done.

+          'stream associated with it. There can be only one listener per '
+          'ScriptProcessorNode');
+    } else {

[Anton Muhin, 2013/03/14 09:30:52]

do we need else clause as then class always throws?  I mean

if (...) {
  throw ...
}

_target = target;
...

[Emily Fortuna, 2013/03/14 18:11:50]

removed else clause

+      _target = target;
+      _createdTargets.add(_target);
+    }
+  }
+
+  bool get isBroadcast => true;
+
+  StreamSubscription<AudioProcessingEvent> listen(
+      void onData(AudioProcessingEvent event),

[Anton Muhin, 2013/03/14 09:30:52]

is onData null allowed?

[Emily Fortuna, 2013/03/14 18:11:50]

no longer. I could have it convert to the no-op function, but right now I'm
having it throw an exception.

+      { void onError(AsyncError error),

[Anton Muhin, 2013/03/14 09:30:52]

nit: no space after {

[Emily Fortuna, 2013/03/14 18:11:50]

Done.

+      void onDone(),
+      bool unsubscribeOnError}) {
+    if (_streamSubscription != null) {
+      throw new StateError('Only one listener is allowed for this particular '
+          'stream.');
+    }
+    _streamSubscription = 
+        new _AudioEventStreamSubscription<AudioProcessingEvent>(
+            this._target, onData);
+    return _streamSubscription;
+  }
+}
+