Making ScriptProcessorNode not an EventTarget.

Reapply making ScriptProcessorNode not an EventTarget.


BUG=

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

[Emily Fortuna, 2012-12-11 01:59:57]

This should be identical to the previous CL:
https://codereview.chromium.org//11469039 I have an email out to Anton and Pavel
to figure out what needs to be done on the dartium side to get this running.
There are some wires we need to hook up on the cpp end/dartium checkout so we
don't turn the dartium bots red.

[blois, 2012-12-11 02:05:17]

lgtm

[Emily Fortuna, 2013-03-13 00:27:57]

PTAL, Pete!

https://codereview.chromium.org/11510013/diff/17001/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/11510013/diff/17001/tests/html/html.status#ne...
tests/html/html.status:24: audiocontext_test: Pass, Timeout, Crash  # Issue
8021.
note: I still need to figure out why this was flaking before checking in this
change, otherwise, my added test will do nothing! :-P

[blois, 2013-03-13 00:46:49]

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:8: void set
onAudioProcess(EventListener listener) {
should probably be void callback(AudioProcessingEvent) rather than
EventListener.

[podivilov, 2013-03-13 07:57:56]

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:8: void set
onAudioProcess(EventListener listener) {
AFAIR we deliberately abandoned attribute event listeners in dart:html.
Can't we implement on.audioProcess.add/remove stuff on top of JS onaudioprocess
setter for consistency with other dart:html API?

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:16:
$!MEMBERS
There's no onAudioProcess getter, right? Is it intentional?

[Emily Fortuna, 2013-03-13 23:35:21]

PTAL

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/17001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:8: void set
onAudioProcess(EventListener listener) {
On 2013/03/13 07:57:56, podivilov wrote:
> AFAIR we deliberately abandoned attribute event listeners in dart:html.
> Can't we implement on.audioProcess.add/remove stuff on top of JS
onaudioprocess
> setter for consistency with other dart:html API?

The reason this can't fit in like all the other event handlers is because
ScriptProcessorNode is not an EventTarget, and we were trying to avoid
reinventing the wheel of Stream providers for this special case where there can
only be one listener. In the newest CL I went ahead and did this though.

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

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:8:
Stream<AudioProcessingEvent> _eventStream;
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:51]

DBC

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:8:
Stream<AudioProcessingEvent> _eventStream;
On 2013/03/14 08:27:39, Anton Muhin wrote:
> please, no: we agreed that we're going to implement simple getter/setter for
> now, let's stick with that, please.

My bad, please, disregard.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:24: class
_AudioEventStreamSubscription<AudioProcessingEvent> extends
why this generic argument?  looks like a typo.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:26:
EventListener _callback;
if we prohibit null _callback, is it true _callback == null iff !_paused?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:31: _paused
= false;
maybe move into field declaration?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:35: void
cancel() {
shouldn't it also clear _AudioEventStream._streamSubscription?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:68: if
(_callback != null) {
if (_callback == null) ?  and has been resumed?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:86: var noop
= (event) {};
why not null?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:87: $if
DART2JS
Why have conditional code here?  Why not have _onAudioProcess in
ScriptProcessorNode itself (with backend-specific implementation) and
backend-independent implementation here?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:109: class
_AudioEventStream<AudioProcessingEvent>
why generic argument here?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:111: static
Set _createdTargets = new Set();
nit: do not we name static _CREATED_TARGETS?  And why omit types?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:116: if
(_createdTargets.contains(target)) {
why we need this check?

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

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:117: throw
new ArgumentError('This ScriptProcessorNode already has an event '
nit: there is a mix of " and ' for string literals.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:120: } else
{
do we need else clause as then class always throws?  I mean

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

_target = target;
...

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:129: void
onData(AudioProcessingEvent event),
is onData null allowed?

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:130: { void
onError(AsyncError error),
nit: no space after {

[Anton Muhin, 2013-03-14 09:32:21]

Last one

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:35: void
cancel() {
cannot we have:

void cancel() { pause(); } ?

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

Okay. I addressed Anton's comments. 

Here's the deal: 
- Stephen and I discussed with Anton and we had decided on the pure
getter/setter scheme. That is Patch 1. 
- Pavel asked if I could do it the other way. Not having heard any positive
feedback from anyone, I implemented it the other way. That's Patch 2/3. 

I don't care which version we commit (Version 1 is a lot simpler, requires less
code, but Version 2 provides (arguably) a better, more consistent API), but
let's please decide on one so I can get this in for our users! 

At this point I have a slight inclination to do version 2 since the initial
argument against that was it was overly complicated code-wise and time-wise to
implement, but now we have it implemented, so we might as well use it.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate
(right):

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:8:
Stream<AudioProcessingEvent> _eventStream;
On 2013/03/14 09:30:52, Anton Muhin wrote:
> On 2013/03/14 08:27:39, Anton Muhin wrote:
> > please, no: we agreed that we're going to implement simple getter/setter for
> > now, let's stick with that, please.
> 
> My bad, please, disregard.

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.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:24: class
_AudioEventStreamSubscription<AudioProcessingEvent> extends
On 2013/03/14 09:30:52, Anton Muhin wrote:
> why this generic argument?  looks like a typo.

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

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:26:
EventListener _callback;
On 2013/03/14 09:30:52, Anton Muhin wrote:
> if we prohibit null _callback, is it true _callback == null iff !_paused?

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

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:31: _paused
= false;
On 2013/03/14 09:30:52, Anton Muhin wrote:
> maybe move into field declaration?

Done.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:35: void
cancel() {
On 2013/03/14 09:32:22, Anton Muhin wrote:
> cannot we have:
> 
> void cancel() { pause(); } ?

Done.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:35: void
cancel() {
On 2013/03/14 09:30:52, Anton Muhin wrote:
> shouldn't it also clear _AudioEventStream._streamSubscription?

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.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:68: if
(_callback != null) {
On 2013/03/14 09:30:52, Anton Muhin wrote:
> if (_callback == null) ?  and has been resumed?

Done.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:86: var noop
= (event) {};
On 2013/03/14 09:30:52, Anton Muhin wrote:
> why not null?

Done.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:87: $if
DART2JS
On 2013/03/14 09:30:52, Anton Muhin wrote:
> Why have conditional code here?  Why not have _onAudioProcess in
> ScriptProcessorNode itself (with backend-specific implementation) and
> backend-independent implementation here?

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/...

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:109: class
_AudioEventStream<AudioProcessingEvent>
On 2013/03/14 09:30:52, Anton Muhin wrote:
> why generic argument here?

removed

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:111: static
Set _createdTargets = new Set();
On 2013/03/14 09:30:52, Anton Muhin wrote:
> nit: do not we name static _CREATED_TARGETS?  And why omit types?

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

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:116: if
(_createdTargets.contains(target)) {
On 2013/03/14 09:30:52, Anton Muhin wrote:
> why we need this check?
> 
> Note as well, that you never remove items from this set which leads to memory
> leaks.

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.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:117: throw
new ArgumentError('This ScriptProcessorNode already has an event '
On 2013/03/14 09:30:52, Anton Muhin wrote:
> nit: there is a mix of " and ' for string literals.

Done.

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:120: } else
{
On 2013/03/14 09:30:52, Anton Muhin wrote:
> do we need else clause as then class always throws?  I mean
> 
> if (...) {
>   throw ...
> }
> 
> _target = target;
> ...

removed else clause

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:129: void
onData(AudioProcessingEvent event),
On 2013/03/14 09:30:52, Anton Muhin wrote:
> is onData null allowed?

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

https://codereview.chromium.org/11510013/diff/37001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_ScriptProcessorNode.darttemplate:130: { void
onError(AsyncError error),
On 2013/03/14 09:30:52, Anton Muhin wrote:
> nit: no space after {

Done.

[Emily Fortuna, 2013-03-14 21:17:09]

PTAL. rewrote as a stream controller.

[Anton Muhin, 2013-03-15 08:00:21]

lgtm,

although I'd rather still have _onAudio setter and the reset implemented on top
of this.

[blois, 2013-03-15 21:37:44]

lgtm

[Emily Fortuna, 2013-03-18 22:18:32]

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