Define the constructor for AudioNodes (#1117) #1144

padenot · 2017-02-08T12:11:13Z

The first commit is the most important of this PR, it defines what happens when constructing AudioNodes (either via the factory method or the ctor). The second fixes the descriptions of the various factory method to link back to the algorithm used (this is mostly a mechanical change). The third patch fixes a mistake in the first patch, and the fourth one defines the constructors in terms of all the above, linking back to the definitions appropriately: it's mostly mechanical, but adjustments have been performed for the AudioWorklet.

rtoy

Still need to review more of the actual constructors, but this is a start.

rtoy · 2017-02-08T23:27:40Z

index.html

+        <p>
+          <a>AudioNode</a>s can be created in two ways: using the constructor
+          for this particular interface, or by using the <dfn>factory
+          method</dfn> on the <a>BaseAudioContext</a>.


Some methods only exist on AudioContext.

rtoy · 2017-02-08T23:29:05Z

index.html

-        </p><a>AudioNode</a>s can be created in two ways:
+        </p>
+        <p>
+          <a>AudioNode</a>s can be created in two ways: using the constructor


Grammar: s/using the constructor/by using the constructor/

rtoy · 2017-02-08T23:30:15Z

index.html

+        <p>
+          To create a new <a>AudioNode</a> of a particular type <var>i</var>
+          using its constructor, with a <a>BaseAudioContext</a> <var>c</var>
+          as first argument, and a <a>associated option object</a>


s/and a associated/and an associated/

rtoy · 2017-02-08T23:33:04Z

index.html

              <li>If the <a>AudioNode</a> being constructed is a
              <a>ConvolverNode</a>, set its <a href=
              "#widl-ConvolverNode-normalize">normalize</a> attribute with the
              inverse of the value of the <a href=
              "#widl-ConvolverOptions-disableNormalization">disableNormalization</a>
-              dictionary member, and then set its the <a href=
+              in <var>dict</var>, and then set its the <a href=


s/its the/its/

rtoy · 2017-02-09T00:05:21Z

index.html

+          in, execute these steps, with <var>k</var> the key of the member, and
+          <var>v</var> its value:
+          <ol>
+            <li>If<var>k</var> is <code>disableNormalization</code> or


rtoy · 2017-02-09T00:06:53Z

index.html

+              optional GainOptions options
+            </dt>
+            <dd>
+              An optional initial gain value for this <a>GainNode</a>.


|options| isn't the actual initial gain value. Not sure how best to describe this. Maybe say nothing at all since there's a link to the description of the actual GainOptions dict? Same goes for all of the following FooOptions dictionaries.

rtoy

Thanks for working on this!

I notice that we're missing constructors for AudioBuffer, MediaStreamTrackAudioSourceNode, and PeriodicWave. I assume these are oversights.

rtoy · 2017-02-15T18:43:28Z

index.html

+          in <var>dict</var>, and then set its <a href=
+          "#widl-ConvolverNode-buffer">buffer</a> attribute to the value of the
+          <a href="#widl-ConvolverOptions-buffer">buffer</a> in <var>dict</var>
+          member, in this order, and jump to the last step of this algorithm.


For the constructor, this ordering isn't really required right? The constructor will look at the disableNormalization and buffer attributes and construct the convolver appopriately.

Yes, this algorithm describes a way of getting the desired result. There are other ways to get the same result.

rtoy · 2017-02-15T18:44:48Z

index.html

-                  the value of the <a href=
-                  "#widl-ConvolverNode-normalize">normalize</a> attribute.
-                </div>
+              <li>If <var>k</var> is the name of an <a>AudioParam</a> on on


Typo: "on on"

rtoy · 2017-02-15T18:45:39Z

index.html

-              method</a> function for this <a>AudioNode</a> type. Initialize
-              all elements of this array to <code>null</code>.
+              <li>If <var>k</var> is <code>disableNormalization</code> or
+              <code>buffer</code>, jump to the beginning of this loop.


AudioBufferSourceNode has a buffer attribute. It looks like we fail to set it in this case.

rtoy · 2017-02-15T18:47:25Z

index.html

+          <li>Let <var>o</var> be a new object of type <var>i</var>.
+          </li>
+          <li>Let <var>option</var> be a dictionnary of the type associated to
+          the interface associated to this factory method.


Need a link for "associated to this factory method" to the definition of "associated interface" for a factor method below.

Done. I also linked more things.

rtoy · 2017-02-15T18:49:57Z

index.html

-        </p><a>AudioNode</a>s can be created in two ways:
+        </p>
+        <p>
+          <a>AudioNode</a>s can be created in two ways: by using the


Editorial: Having this be a subsection is nice so that we can get a link to this entire algorithm.

rtoy · 2017-02-15T18:52:07Z

index.html

@@ -4269,6 +4331,31 @@ <h2 id="GainNode">
        "[Constructor(&lt;a&gt;BaseAudioContext&lt;/a&gt; context, optional &lt;a&gt;GainOptions&lt;/a&gt; options)]interface GainNode : &lt;a&gt;AudioNode&lt;/a&gt;"
        class="idl">


Something strange is happening here. In both Chrome and Firefox, I see the "Constructor" listed twice. Do you see this @padenot? And it seems to be happening for all nodes.

apparently when you define it in the <dl>, then you don't have to do it in the title. I'll fix.

rtoy · 2017-02-15T18:58:08Z

index.html

+            <p>
+              Let <var>gain</var> be a new <a>GainNode</a> object. <a href=
+              "#audionode-constructor-common">Initialize the common part</a> of
+              <var>gain</var>, and return <var>gain</var>.


I find this a bit confusing. You say the common part is initialized. What about the non-common parts?

This is not called "initializing an AudioNode", hopefully less confusing.

rtoy · 2017-02-15T19:00:07Z

index.html

+        </ol>
+        <p>
+          <dfn id="audionode-constructor-common">Initializing the common
+          part</dfn> of an object <var>o</var> of interface <var>i</var> that


This talks about initializing the common parts like the number of inputs/outputs and channel parameters, and it does. But then it also talks about the non-common parts like the node-specific options.

I think this needs to be split up into two pieces or just renamed to so that's not about initializing the common part.

I've renamed it, you're right.

rtoy · 2017-02-15T19:03:04Z

index.html

+                be <a href="#associated">associated</a> with.
+              </dd>
+              <dt>
+                optional IIRFilterOptions options


This isn't optional.

rtoy · 2017-02-15T19:03:57Z

index.html

+                "#associated">associated</a> with.
+              </dd>
+              <dt>
+                optional MediaStreamAudioSourceOptions options


This isn't optional.

This fixes WebAudio#1117.

… link to it

…finition issue.

padenot · 2017-03-20T17:37:53Z

@rtoy, sorry for the delays, I've addressed the issues you found, preview here.

rtoy

Looks good, but it seems we're missing AudioBuffer and PeriodicWave constructors.

rtoy · 2017-03-20T19:22:46Z

index.html

+        </p>
+        <section>
+          <h3>
+            AudioNode creation


Nit: "creation" should be capitalized.

rtoy · 2017-03-20T19:24:37Z

index.html

+            "associated">associated <a>BaseAudioContext</a></dfn> of the
+            <a>AudioNode</a> to be created. Similarly, when using the factory
+            method, the <a>associated <code>BaseAudioContext</code></a> of the
+            <a>AudioNode</a> is the first argument of the factory method.


This seems wrong since many factory methods don't have a first argument at all. Or perhaps I don't understand what you're trying to say here.

Yeah this is wrong. I meant "The associated BaseAudioContext when calling a factory method is the BaseAudioContext this factory method is called on", which makes sense.

rtoy · 2017-03-20T19:26:25Z

index.html

+            <a>AudioNode</a> is the first argument of the factory method.
+          </p>
+          <p>
+            To create a new <a>AudioNode</a> of a particular type <var>i</var>


Can we use "n" instead of "i"? I find it hard to distinguish between an "i" and a "l" and a "1".

rtoy · 2017-03-20T19:29:44Z

index.html

+            </li>
+            <li>
+              <a href="#audionode-constructor-init">Initialize the common
+              part</a> of <var>o</var>, with <var>c</var> and <var>option</var>


I assume you don't want to say "Initializing the common part" here anymore because that section isn't about just the common part anymore.

Yeah I half assed my regex apparently. Fixed.

rtoy · 2017-03-20T19:30:08Z

index.html

+          <ol>
+            <li>Let <var>o</var> be a new object of type <var>i</var>.
+            </li>
+            <li>Let <var>option</var> be a dictionnary of the type <a href=


Typo: "dictionnary" -> "dictionary"

rtoy · 2017-03-20T19:31:05Z

index.html

+            value of this parameter.
+            </li>
+            <li>
+              <a href="#audionode-constructor-init">Initialize the common


Ditto. It's not the common part anymore.

rtoy · 2017-03-20T19:36:03Z

index.html

-        <dl title=
-        "[Constructor(&lt;a&gt;AudioBufferOptions&lt;a&gt; options)]interface AudioBuffer"
-        class="idl">
+        <dl title="interface AudioBuffer" class="idl">


I think you forget to define the actual constructor for this. And the constructor for this doesn't follow the AudioNode constructors because context is not the first arg. Not sure where the best place to describe the constructor should be.

Those are not AudioNodes, the goal is to fix it in a different PR (this PR and issue is about AudioNodes, there are other issues about other objects).

This is different because the notion of "associated AudioContext" does not apply, among other things. I've reinstated this constructor.

Anyways, I plan on following up immediately with defining the ctors for other objects, this PR is big enough already.

rtoy · 2017-03-20T19:38:15Z

index.html

+                "#associated">associated</a> with.
+              </dd>
+              <dt>
+                optional MediaElementAudioSourceOptions options


This is not optional.

rtoy · 2017-03-20T19:41:01Z

index.html

+              </dd>
+            </dl>
+          </dd>
+        </dl>


Something is messed up somewhere. The Media* nodes all used to be in successive sections. Now the worklet node comes after MediaElementAudioSource, and the other nodes are all placed at the end.

This PR does not change this, but I can do a patch to reorder the section if you'd like.

If you haven't already, don't bother. Recall that we're going to reorder all the sections anyway to roughly alphabetical.

rtoy · 2017-03-20T19:44:38Z

index.html

+                IIRFilterOptions options
+              </dt>
+              <dd>
+                Optional initial parameter value for this <a>IIRFilterNode</a>.


Not optional.

rtoy · 2017-03-21T15:13:53Z

For those following along: https://rawgit.com/WebAudio/web-audio-api/591f0a7/index.html

rtoy

lgtm. Thanks for working on this!

rtoy reviewed Feb 9, 2017

View reviewed changes

rtoy reviewed Feb 15, 2017

View reviewed changes

joeberkovitz assigned padenot Mar 16, 2017

padenot added 11 commits March 20, 2017 17:06

Define precisely how constructors and factory method work.

49ae0c4

This fixes WebAudio#1117.

Fix the ctor descriptions

a7a2cea

The first argument of a factory method is not a BaseAudioContext

1a334f3

Define the ctor of all the AudioNodes.

95f141e

Address rtoy comments.

72bf5a2

Run tidy

3aba3bd

Add a ctor for MediaStreamTrackAudioSourceNode

975f13d

Put the section about AudioNode creation in its own section so we can…

268b100

… link to it

Link the terme assocatiated properly to their definitions

ffbe9c2

Rename the section about initializing the common part of a node

f36a5d8

Remove 'optional' keyboards in some ctors, and fix the double-ctor-de…

a338555

…finition issue.

padenot force-pushed the 1117-constructors-take-two branch from de7d3ef to a338555 Compare March 20, 2017 17:36

Run tidy

ecb50a9

rtoy requested changes Mar 20, 2017

View reviewed changes

Address rtoy comments and re-run tidy.

591f0a7

padenot force-pushed the 1117-constructors-take-two branch from 9c37403 to 591f0a7 Compare March 21, 2017 15:12

rtoy approved these changes Mar 21, 2017

View reviewed changes

padenot merged commit e6eea8a into WebAudio:gh-pages Mar 21, 2017

		@@ -4269,6 +4331,31 @@ <h2 id="GainNode">
		"[Constructor(<a>BaseAudioContext</a> context, optional <a>GainOptions</a> options)]interface GainNode : <a>AudioNode</a>"
		class="idl">

Define the constructor for AudioNodes (#1117) #1144

Define the constructor for AudioNodes (#1117) #1144

Conversation

padenot commented Feb 8, 2017

rtoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padenot commented Mar 20, 2017

rtoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtoy commented Mar 21, 2017

rtoy left a comment

Choose a reason for hiding this comment