Better description of what AudioNode Constructors should do

[bakulf]

AudioNodes have two ways to be constructed: Constructors and Factory methods.
We should definitely describe better what the Constructors should do because sometimes, this is unclear.
If these Constructors are supposed to call factory methods, I suggest to see how each param should be passed.

[bzbarsky]

I suspect the sane way to define this would be to:

1. Define the constructor behavior in detail, probably by moving over the current definition of the factor method.
2. Define that the factory method calls the constructor.

That's assuming the arguments to the two are meant to be interpreted the same way, of course.

[padenot]

Since this is the same for each constructor, we specced it directly in the base class (AudioNode): https://webaudio.github.io/web-audio-api/#dfn-factory-method.

We did it the opposite way, the constructor calls the factory method, speccing how different things (attributes, AudioParams, etc.) should be set and a special case for the ConvolverNode.

[bzbarsky]

Since this is the same for each constructor, we specced it directly in the base class (AudioNode): https://webaudio.github.io/web-audio-api/#dfn-factory-method.

That's totally not discoverable from reading the actual definition of an AudioNode subclass constructor. At the very least, those should link to the definition here. However....

We did it the opposite way, the constructor calls the factory method

There are several problems with doing it this way:

1. The way you have it written, nothing actually uses the AudioParams you are setting up in https://webaudio.github.io/web-audio-api/#dfn-factory-method step 2.3.1, as far as I can tell.

2. The attributes being set in https://webaudio.github.io/web-audio-api/#dfn-factory-method step 2.3.2 are being set on an object that hasn't been created yet; it will be created in step 2.4. This is pretty confusing.

3. At first glance the algorithm can end up passing null where it's not allowed to be passed to the factory method. Maybe all the options dictionaries enforce that the non-nullable arguments of the factory method are required dictionary properties, but that's pretty fragile.

4. The result violates the Web IDL spec. Per IDL spec, a constructor call produces an object associated with the global of the constructor. But your constructor definition creates the objects via factory method on the audiocontext, which might be from a different global.

Consider this testcase:

 <iframe></iframe>
 <script>
   var ctx = new AudioContext();
   var node = new frames[0].WaveShaperNode(ctx);
   alert(Object.getPrototypeOf(node) == frames[0].WaveShaperNode.prototype)
 </script>

This should alert true per WebIDL spec and sanity, but afaict false per current WebAudio spec. In the Firefox implementation, as far as I can tell from code inspection, it will alert true, due to details of our binding implementation, but after some GC it can start alerting false(!). And this didn't get caught in review, because the reviewer had now idea how this stuff should behave, based on the spec.

5. There are ways to create an object via the constructor that are not possible via the factory method.

Specifically, consider this:

<script>
  var ctx = new AudioContext();
  class MyNode extends WaveShaperNode {
    constructor(ctx) {
      super(ctx);
    }
  }
  var node = new MyNode(ctx);
  alert(Object.getPrototypeOf(node) == MyNode.prototype)
</script>

this should alert true, but per webaudio spec I don't see why it would, since the constructor is invoking some totally unrelated thing to create the object, which doesn't know about the newTarget.

6. It makes spec editors sloppy and they start not defining constructors for non-AudioNode things either. I just realized that PeriodicWave, which is why I started looking at this stuff to start with, isn't even an AudioNode! I filed PeriodicWave constructor behavior is not defined #1120 on that.

The sane way to do all this is to have a set of steps the constructor performs to initialize the object after creating it. The factory method can either invoke those same steps, by factoring them out into a spec algorithm invoked from both places, or call the constructor from the appropriate global. The latter is slightly preferable, because it centralizes the "how is the object actually created?" magic.

[bzbarsky]

To be clear, there is no problem in general with having some sort of centralized processing steps each constructor calls (though the particular steps in the spec have issues 1, 2, 3 above). But those steps need to be discoverable from the constructor definition. And in particular, the word "Constructor" in the IDL needs to link to those steps!

[padenot]

Right. I'll reverse it here (make the factory method call the ctor). Thanks for the details, hopefully we can sort all this out soon.

[padenot]

@bzbarsky, https://rawgit.com/padenot/web-audio-api/1117-constructors-take-two/index.html has a work in progress. Since this is going to be quite some work, it would be best to at least know if I'm going in the right direction here.

Specifically:

• The factory method are now using the constructor, creating the option dictionary on the fly. This has to do some introspection on the attribute name to create the option dictionary, hopefully that's OK.
• There is an algorithm that factors the common part of initializing an AudioNode. It is defined more or less in the same way the factory method used to be defined.
• I've converted the GainNode to the new way of defining the constructor: the factory method (createGain) description links to the algorithm to use for factory methods, its constructor links to the algorithm to run for AudioNode constructors.
• Some attributes of the AudioNode interface are now initialized in the constructor (channel{Count,CountMode,Interpretation} and numberOf{Inputs,Outputs}).

Now the different problems:

• Historically, the spec was created without the distinction between objects slots and attributes on the IDL, so we're using prose like "set the attribute X to the value v", which is incorrect. I've been told new syntax (or something else) is on the way so that we could spec this without having to be very verbose (Adding slots [initial braindump] whatwg/webidl#258). At the moment, we're using the attributes throughout the spec as something that we can set and get, instead of having some value on an internal concept that we use.
• Similarly, we're calling into the binding from the binding itself (for example, when we're setting an AudioParam value from the constructor, this calls the setter for the value attribute, which calls setValueAtTime for this AudioParam).
• I'm not too sure how to "call the constructor from the appropriate global"

[bzbarsky]

This has to do some introspection on the attribute name to create the option dictionary

Why? Why can't you just pass the type of dictionary involved, or even the dictionary itself, to the "factory method" algorithm from the various callsites?

The "initializing the common part" text has some weird phrasing in step 1. Maybe you mean:

Set _o_'s associated BaseAudioContext to context. 

? Or:

Set its associated BaseAudioContext to context.

I haven't had a chance yet to read the proposed changes, but as far as the problems being mentioned here...

Else if k is the name of an attribute on this interface, set the object associated with this attribute to v.

Should that be "set the value associated with this attribute"? Does it have to be an object?

But in general I think this looks sane.

At the moment, we're using the attributes throughout the spec as something that we can set and get

This is not a new issue, right?

Similarly, we're calling into the binding from the binding itself

I'm not sure I follow...

for example, when we're setting an AudioParam value from the constructor, this calls the setter for the value attribute, which calls setValueAtTime for this AudioParam

This seems fine at first glance., but maybe I'm missing something?

I'm not too sure how to "call the constructor from the appropriate global"

Maybe something like this: "Call the FooNode constructor from the relevant global", where "relevant global" links to https://html.spec.whatwg.org/multipage/webappapis.html#concept-relevant-global

It's a bit weird, because we want to pass an actual dictionary, not a JS object to be converted to a dictionary. @annevk do we have any better existing ideas on how this sort of thing should be structured?

[annevk]

It's best not to invoke the constructor, since that is the public API. Just define an algorithm that creates and initializes the object. Maybe once IDL defines object creation in more detail there will be a better way.

[bzbarsky]

And call that algorithm from both the factory method and the constructor?

[annevk]

Yeah. See https://url.spec.whatwg.org/#concept-urlsearchparams-new for instance.

[padenot]

I've updated the text somewhat. There is now two algorithms (one for the ctor and one for the factory method), and they use the same an abstract operation to initialize the AudioNode, fixing various other things in the process (based on all the comments here), last version at https://rawgit.com/padenot/web-audio-api/1117-constructors-take-two/index.html#the-audionode-interface

@domenic, I've been asked by Boris to ask you about which global to use for:

• The newly constructed object, created using the constructor. We say something like "Let o be a new SomeInterface object". Should we specify a particular global here, or is it implicit? If we have to be explicit, is taking the same gloabal as the first argument of the constructor (That is always a BaseAudioContext), considering that all AudioNodes belong to a context ?
• The newly constructed object, created using the factory method. My current version also says to use the global of the associated BaseAudioContext.

You can search for "AudioNodes can be created in two ways" in the link above to jump directly to the relevant text.

Thanks,
Paul.

[bzbarsky]

For what it's worth, the global of the object constructed by the constructor is already defined by the IDL spec: it's the global of the Realm of the constructor function.

The only real question is what the factory method does.

[domenic]

Right, the constructor is implicit.

For the factory method, I guess we've settled on indeed using the same global/Realm as the object on which the method is being called, i.e. the relevant global/Realm of this BaseAudioContext object.

[padenot]

Thanks both, I can finish this now.