1458446 - Add AudioWorkletNode interface definitions

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement, P3)

Description

[Karl Tomlinson (:karlt)]

Something like https://reviewboard.mozilla.org/r/86424/diff/
but for AudioWorkletNode.

https://reviewboard.mozilla.org/r/199446/diff/ may be a start.

Comment 1

[Arnaud Bienner]

I've started working on this:
I've updated the following webidl (and their corresponding implementations):
* AudioWorkletGlobalScope.webidl
* EventTarget.webidl
* MessagePort.webidl
since their specifications were updated too.

And added the following new webidl files:
* AudioParamMap.webidl
* AudioWorkletNode.webidl
* AudioWorkletProcessor.webidl

However I'm not sure how to implement the corresponding classes.
Looking around, I've found some similar classes which subclass different interfaces, and implement different set of methods.
And I'm not sure what should be done for each of those new webidl.

If someone has any ideas or pointers, I would be interested.

Karlt: any advice?

Comment 2

[Karl Tomlinson (:karlt)]

The best way to move something forward is usually to break the task up into
smaller chunks, and usually smaller is better than larger.

If the modifications to existing webidl files can build on their own, then
prepare those changes in separate patches.  If you submit them in a different
bug, then I expect they can land before the others are complete.

For the others, similarly look for the smallest step that can build.  If
methods are at all non-trivial, then provide no-op implementations or throw an
error indicating not-implemented for now.

Perhaps leaving some of the methods out of the webidl initially may also be an
option if returning null or throwing is not possible for some reason, or there
are missing dependencies.  If doing that, then a comment to indicate what's
missing would be helpful.

For AudioWorkletNode, I expect the patch for this bug will look something like
https://reviewboard.mozilla.org/r/86424/diff/2/

I guess AudioWorkletNode needs AudioParamMap, and so it may be easiest to add
AudioParamMap on its own first.

AudioWorkletProcessor is different to anything that has been implemented
before for Web Audio, and so it may be best to leave that for another bug.

The documentation at
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings and
specifically `mach webidl-example MyInterface` will be useful.

Comment 3

[Arnaud Bienner]

Thanks Karl.
Since this seems more complicated than what I initially thought, I believe you're right about splitting this in multiple parts.
I've started updating AudioWorkletGlobal definitions in a separate bug (bug 1460896).

Comment 4

[Arnaud Bienner]

I'm unsure if it is best to have the corresponding C++ class in dom/worklet or dom/media/webaudio

I feel like having it in dom/media/webaudio makes more sense, and keep all common worklet stuff in dom/worklet.
If we go that way, we might want to move AudioWorkletGlobalScope to webaudio too.

Karlt: any advice?

Comment 5

[Arnaud Bienner]

Also, which classes do you think AudioWorkletNode should subclass?
I'm looking at examples around (including existing AudioNode class) but I'm not sure what is best, since I'm not sure what each subclassed-classes are doing, and what would be required in our case.

Comment 6

[Arnaud Bienner]

Actually, subclassing AudioNode is probably the way to go (since the WebIDL does).
Not sure yet if I need to subclass something else, and if there are others methods I should reimplement (except Constructor which is redefined in AudioWorkletNode WebIDL).

Comment 7

[Arnaud Bienner]

Attachment: Bug 1458446 - Add AudioWorkletNode interface definitions.

Review commit: https://reviewboard.mozilla.org/r/248566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248566/

Comment 8

[Arnaud Bienner]

Removing the needinfo since I think we can discuss about the questions I had over the code review if necessary.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #4)
> I'm unsure if it is best to have the corresponding C++ class in dom/worklet
> or dom/media/webaudio
> 
> I feel like having it in dom/media/webaudio makes more sense, and keep all
> common worklet stuff in dom/worklet.

Yes, dom/media/webaudio is good for AudioWorkletNode at least, and I think
AudioWorkletProcessor too.

> If we go that way, we might want to move AudioWorkletGlobalScope to webaudio
> too.

Yes, it would be sensible to implement currentFrame, currentTime and
sampleRate in dom/media/webaudio.

Perhaps it may be easier to implement RegisterProcessor() in dom/worklet than
dom/media/webaudio, but I think it makes more sense to use any necessary
worklet-global-scope-related objects from dom/worklet in a derived class in
dom/media/webaudio, than to use web-audio-specific objects in dom/worklet.

dom/media/webaudio does seem better for AudioWorkletGlobalScope.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review255356

Add a AudioWorkletNode entry to test_interfaces and, when you are ready, request review from :baku, as he has done several of these.

::: dom/media/webaudio/AudioWorkletNode.h:33
(Diff revision 1)
> +  Constructor(const GlobalObject& aGlobal,
> +              AudioContext& aContext,
> +              const nsAString& aName,
> +              const AudioWorkletNodeOptions& aOptions,
> +              ErrorResult& aRv);
> +

Need a WrapObject() override to call the correct binding.

(Inheriting GetParentObject() from AudioNode is good.)

::: dom/media/webaudio/AudioWorkletNode.h:37
(Diff revision 1)
> +              ErrorResult& aRv);
> +
> +  AudioParamMap* Parameters() const;
> +
> +  MessagePort* Port() const;
> +};

The destructor of a ref-counted object should be private.

::: dom/media/webaudio/AudioWorkletNode.h:37
(Diff revision 1)
> +              ErrorResult& aRv);
> +
> +  AudioParamMap* Parameters() const;
> +
> +  MessagePort* Port() const;
> +};

We'll need overrides for NodeType(), SizeOfExcludingThis(), SizeOfIncludingThis()

::: dom/media/webaudio/AudioWorkletNode.cpp:11
(Diff revision 1)
> +
> +#include "AudioWorkletNode.h"
> +
> +namespace mozilla {
> +namespace dom {
> +

NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED would be appropriate for this class I expect.

(IIUC NS_INLINE_DECL_REFCOUNTING_INHERITED would not be appropriate because the object will end up holding a strong references to the port
and parameters, which will need to be traversed for cycle collection.)

::: dom/media/webaudio/AudioWorkletNode.cpp:11
(Diff revision 1)
> +
> +#include "AudioWorkletNode.h"
> +
> +namespace mozilla {
> +namespace dom {
> +

A private AudioWorkletNode C++ constructor could be defined to pass appropriate AudioNode() parameters.

::: dom/webidl/AudioWorkletNode.webidl:21
(Diff revision 1)
> +[Pref="dom.audioworklet.enabled",
> + Constructor (BaseAudioContext context, DOMString name, optional AudioWorkletNodeOptions options)]

SecureContext should be in here.

::: xpcom/ds/nsGkAtomList.h:768
(Diff revision 1)
>  GK_ATOM(onpopstate, "onpopstate")
> +GK_ATOM(onprocessorerror, "onprocessorerror")
>  GK_ATOM(only, "only")               // this one is not an event

onpopstate seems to be out of order here.
I would add the new entry after onpopupshown.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review255694

::: dom/media/webaudio/AudioWorkletNode.h:11
(Diff revision 1)
> +#include "AudioParamMap.h"
> +#include "mozilla/dom/AudioWorkletNodeBinding.h"
> +#include "mozilla/dom/MessagePort.h"

Aim to minimize the number of headers included from header files by forward declaring in the header file and including only in the C++ file.  This reduces the number of files that need recompiling when one header changes.

I'm guessing AudioWorkletNodeBinding is not actually used in the header file, and it should be possible to forward declare AudioParamMap and MessagePort.

Comment 12

[Arnaud Bienner]

I wasn't sure yet which headers I would really need so I was thinking about cleaning that later.
Also, I've been using fwd-decl on all my projects for a while, indeed to reduce compil time. On some of them we were following Google coding rules, which used to recommend fwd-dcl. But that changed recently and now they recommend using include instead to avoid weird behavior in some cases [1], and they made good points.
So I feel like it's safer to always use include, and use fwd-decl only for special cases (e.g. when implementing PIMPL idiom).

Here I believe it's probably safe to fwd-decl, so I can change that if you really prefer (unless the remarks I made changed your mind).

[1]: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

Comment 13

[Arnaud Bienner]

(In reply to Karl Tomlinson (:karlt) from comment #10)
> We'll need overrides for NodeType(), SizeOfExcludingThis(),
> SizeOfIncludingThis()

What do you think NodeType should return? I guess it depends on how it is used.
I believe it should return const char* "AudioWorkletNode" similarly to what ScriptProcessor does.
However, since AWN have names, I was wondering if that name should be returned instead.
If so, that might be an issue since the name is a DOMString i.e. UTF-16 string, and I'm not sure if NodeType should return ASCII only.

Regarding SizeOfIn/ExcludingThis functions, I'm not sure what is required here. Would a simple function (like what ScriptProcessor does) be enough?

> ::: dom/webidl/AudioWorkletNode.webidl:21
> (Diff revision 1)
> > +[Pref="dom.audioworklet.enabled",
> > + Constructor (BaseAudioContext context, DOMString name, optional AudioWorkletNodeOptions options)]
> 
> SecureContext should be in here.

Sorry, I 'm not sure I understood what you meant here.

Comment 14

[Arnaud Bienner]

Looking at the AWN spec [1] I see numberOfInput/Outputs defaults to 1 (but can be modified using AudioWorkletNodeOptions) but channel* parameters (including channelCount) can't be, and default to 2.
Does that mean we AWN channel inputs should always stereo?
If no, how does the node will figure out the number of input channels?

[1]: https://webaudio.github.io/web-audio-api/#audioworkletnode

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #14)
> Looking at the AWN spec [1] I see numberOfInput/Outputs defaults to 1 (but
> can be modified using AudioWorkletNodeOptions) but channel* parameters
> (including channelCount) can't be, and default to 2.
> Does that mean we AWN channel inputs should always stereo?

No.

> If no, how does the node will figure out the number of input channels?

The client can modify the channel* properties on the node (for most node types).

https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/dom/media/webaudio/AudioNode.h#132

The number of channels from the upstream node can also be involved in the
calculation of computedNumberOfChannels.

https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/dom/media/webaudio/AudioNodeStream.cpp#361

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #13)
> What do you think NodeType should return? I guess it depends on how it is
> used.
> I believe it should return const char* "AudioWorkletNode" similarly to what
> ScriptProcessor does.

Yes.  Do that.

> However, since AWN have names, I was wondering if that name should be
> returned instead.
> If so, that might be an issue since the name is a DOMString i.e. UTF-16
> string, and I'm not sure if NodeType should return ASCII only.

Although UTF-16 could be converted to UTF-8, the pointer needs to reference a
string from constant static storage because AudioNodeEngines have a copy of
this pointer and lifetime potentially longer than their AudioNodes.

Although the name could be useful, NodeType() does not support non-static
strings at this stage.

https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/dom/media/webaudio/AudioNodeEngine.cpp#387

> Regarding SizeOfIn/ExcludingThis functions, I'm not sure what is required
> here. Would a simple function (like what ScriptProcessor does) be enough?

Yes, initially the methods will look like those for ScriptProcessor.

As more objects are owned by the node, they will be included in
SizeOfExcludingThis().

> > ::: dom/webidl/AudioWorkletNode.webidl:21
> > (Diff revision 1)
> > > +[Pref="dom.audioworklet.enabled",
> > > + Constructor (BaseAudioContext context, DOMString name, optional AudioWorkletNodeOptions options)]
> > 
> > SecureContext should be in here.
> 
> Sorry, I 'm not sure I understood what you meant here.

The "SecureContext" extended attribute needs to be specified for the
AudioWorkletNode interface.

grep SecureContext dom/webidl/* for examples.

Comment 17

[Arnaud Bienner]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248566/diff/1-2/

Comment 18

[Arnaud Bienner]

I've updated the patch based on your comments.
I had to rebase (compilation was failing because of a recent change on central), so nsGkAtomList.h is messed up, sorry for that :/

There are a few things I'm not sure about.
I'm not familiar with cycle collection model, but I thought I might have need to use NS_IMPL_CYCLE_COLLECTION_INHERITED or something similar with the owned objects passed as parameters (MessagePort and AudioParamMap once we have them), so cycle collection knows about them.
BTW we might add RefPtr<MessagePort> and RefPtr<AudioParamMap> class members as part of this patch, even if they aren't initialized, what do you think?
But If we do, I believe we would need more complex SizeOf functions, and I'm not sure how they will look like, given that nor MessagePort or AudioParamMap implement SizeOf functions.

I'm not sure if everything I said makes sense: there are lot of things I'm not familiar with here :)

Comment 19

[Karl Tomlinson (:karlt)]

We will need RefPtr<MessagePort> and RefPtr<AudioParamMap> at some point.
They can either be added now or later, even without the SizeOf functions.

I think you are right that NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED will
need to be replaced for the port and AudioParamMap.  The parameters for
NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED are all for the interface table,
so the port and param map will need NS_IMPL_CYCLE_COLLECTION_INHERITED, as you
say.  I don't mind whether you'd like to add that now or later

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review257622

::: dom/media/webaudio/AudioWorkletNode.h:50
(Diff revisions 1 - 2)
> +  size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const override;
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const override;
> +
> +private:
> +  AudioWorkletNode(AudioContext* aAudioContext, const nsAString& aName);
> +  ~AudioWorkletNode() = default;

If adding nsRefPtr members with forward declared types, move the definition to the .cpp file.

::: dom/media/webaudio/AudioWorkletNode.cpp:36
(Diff revisions 1 - 2)
> +      return nullptr;
> +    }

need to correct alignment

::: dom/media/webaudio/AudioWorkletNode.cpp:39
(Diff revisions 1 - 2)
>  {
> +  if (aAudioContext.CheckClosed(aRv)) {
> +      return nullptr;
> +    }
> +
> +  if (aOptions.mNumberOfInputs == 0 ||

mNumberOfInputs == 0 should not be disallowed if mNumberOfOutputs is non-zero.

That would indicate a source node.

If both mNumberOfInputs and mNumberOfOutputs are zero, then the spec indicates NotSupportedError.

::: dom/media/webaudio/AudioWorkletNode.cpp:40
(Diff revisions 1 - 2)
> +      aOptions.mNumberOfInputs > WebAudioUtils::MaxChannelCount) {
> +    aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);

I don't see a particular error specified for an unsupported non-zero number of inputs, but NS_ERROR_DOM_NOT_SUPPORTED_ERR seems better in the case of an unspecified maximum and would be consistent with the case with zero inputs and outputs.

::: dom/tests/mochitest/general/test_interfaces.js:162
(Diff revision 2)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "AudioScheduledSourceNode", insecureContext: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "AudioStreamTrack", insecureContext: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "AudioWorkletNode", insecureContext: true, disabled: true},

I expect this should not have insecureContext set, because it is available only in secure contexts.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review257642

I'm asking Andrea for review on the webidl parts.

Comment 22

[Arnaud Bienner]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review257622

> I don't see a particular error specified for an unsupported non-zero number of inputs, but NS_ERROR_DOM_NOT_SUPPORTED_ERR seems better in the case of an unspecified maximum and would be consistent with the case with zero inputs and outputs.

Actually I was wrong here.
The number of inputs isn't related to the number of channels.
Looking at the spec, I don't see any limit on the number of inputs.

There is however some cases to be taken into into account that I missed:
- "numberOfInputs = 0, numberOfOutputs = 0 NotSupportedError MUST be thrown by the constructor" as you noted
- "IndexSizeError MUST be thrown if the length of sequence [outputChannelCount] does not match numberOfOutputs."
- "NotSupportedError exception MUST be thrown if a channel count is not in the valid range of AudioNode’s channelCount."

I've implemented those missing checks.

> I expect this should not have insecureContext set, because it is available only in secure contexts.

I changed that, but the test always successed, no matter if disabled is false or if the pref is removed in the webidl part (unlike when insecureContext == true), which makes me think nothing is tested if insecureContext == false.
Looking at createInterfaceMap::addInterfaces, around line 1268 we do `interfaceMap[entry] = !isInsecureContext;` so I suspect those tests are indeed disabled for now.
But let me know if I missed anything.

Comment 23

[Arnaud Bienner]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248566/diff/2-3/

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review257622

> I changed that, but the test always successed, no matter if disabled is false or if the pref is removed in the webidl part (unlike when insecureContext == true), which makes me think nothing is tested if insecureContext == false.
> Looking at createInterfaceMap::addInterfaces, around line 1268 we do `interfaceMap[entry] = !isInsecureContext;` so I suspect those tests are indeed disabled for now.
> But let me know if I missed anything.

`interfaceMap[entry] = !isInsecureContext;` is only when `typeof(entry) ===
"string"`, which should not be true here.

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/dom/tests/mochitest/general/test_interfaces.js#1265

I wonder whether you've been running dom/tests/mochitest/general/test_interfaces.html
but not dom/tests/mochitest/general/test_interfaces_secureContext.html

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review258120

::: dom/media/webaudio/AudioWorkletNode.cpp:39
(Diff revisions 2 - 3)
>  {
>    if (aAudioContext.CheckClosed(aRv)) {
> -      return nullptr;
> +    return nullptr;
> -    }
> +  }
>  
> -  if (aOptions.mNumberOfInputs == 0 ||
> +  if (aOptions.mOutputChannelCount.Value().Length() !=

Value() is required because mOutputChannelCount is optional.

WasPassed() must be checked before calling Value().

I suggest moving this block to after the aOptions.mNumberOfInputs == 0 && aOptions.mNumberOfOutputs == 0 block so that this block and the for loop can share a single WasPassed() test.

::: dom/media/webaudio/AudioWorkletNode.cpp:51
(Diff revisions 2 - 3)
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return nullptr;
> +  }
> +
> +  for (uint32_t channelCount : aOptions.mOutputChannelCount.Value()) {
> +    if (channelCount > WebAudioUtils::MaxChannelCount) {

Throw also for channelCount == 0, because 0 is not a valid channelCount.

Comment 26

[Arnaud Bienner]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248566/diff/3-4/

Comment 27

[Arnaud Bienner]

You're right about test_interfaces_secureContext.html, thanks for pointing this out :)

I updated the patch based on your comment.

But thinking again about this, I wonder if having optional checks based on WasPassed is really the way to go: AudioWorkletNodeOptions is optional, but within AudioWorkletNodeOptions, outputChannelCount is not.

I'm not sure how to check this in the code, since we don't have a WasPassed method at AudioWorkletNodeOptions level.
Also, outputChannelCount's C++ generated type is Optional<Sequence<uint32_t>>, so probably I just misunderstood something, since this is what was generated from the webidl.

Side note: one thing I noticed, related to the spec: why do we have both numberOfOutputs and outputChannelCount?
That looks redundant, since numberOfOutputs is supposed to be equal to outputChannelCount's size, why not just rely on outputChannelCount's to know the number of output?

Comment 28

[Arnaud Bienner]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

Re-add r? flag

Comment 29

[Karl Tomlinson (:karlt)]

The latest changes are good, thanks.

(In reply to Arnaud Bienner from comment #27)
> But thinking again about this, I wonder if having optional checks based on
> WasPassed is really the way to go: AudioWorkletNodeOptions is optional, but
> within AudioWorkletNodeOptions, outputChannelCount is not.

https://heycam.github.io/webidl/#idl-dictionaries says "On a given dictionary
value, the presence of each dictionary member is optional, unless that member
is specified as required" and "Dictionary members with default values are
always considered to be present."
 
> I'm not sure how to check this in the code, since we don't have a WasPassed
> method at AudioWorkletNodeOptions level.

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types
says "Note that optional dictionary arguments are always considered to have a
default value of null, so dictionary arguments are never wrapped in
Optional<>".  "A dictionary argument is represented by a const reference
[...]" and so C++ cannot see a null pointer.  However, when a JS null argument
is converted to a webidl dictionary, the result is a dictionary with undefined
or default values.
https://heycam.github.io/webidl/#es-dictionary

So yes, I don't know how to determine whether the AudioWorkletNodeOptions
parameter was passed, but I don't think that needs to be determined.

> Also, outputChannelCount's C++ generated type is
> Optional<Sequence<uint32_t>>, so probably I just misunderstood something,
> since this is what was generated from the webidl.

This is documented and consistent with the example: "The members that are not
required and don't have default values have those types wrapped in
Optional<>."

> Side note: one thing I noticed, related to the spec: why do we have both
> numberOfOutputs and outputChannelCount?
> That looks redundant, since numberOfOutputs is supposed to be equal to
> outputChannelCount's size, why not just rely on outputChannelCount's to know
> the number of output?

Yes, numberOfOutputs is redundant AFAICT.

Apparently numberOfOutputs exists solely to provide a way of configuring
multiple mono outputs without explicitly specifying outputChannelCount.

Comment 30

[Karl Tomlinson (:karlt)]

I changed the docs because null is not a valid webidl dictionary value.
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings$compare?locale=en-US&to=1389772&from=1367644

Comment 31

[Andrea Marchesini [:baku]]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

https://reviewboard.mozilla.org/r/248566/#review258430

I want to review the following patches where you create the MessagePort and you expose it. Thanks!

::: dom/media/webaudio/AudioWorkletNode.cpp:71
(Diff revision 4)
> +  }
> +
> +  return audioWorkletNode.forget();
> +}
> +
> +AudioParamMap* AudioWorkletNode::Parameters() const

See the MessagePort comment.

::: dom/media/webaudio/AudioWorkletNode.cpp:77
(Diff revision 4)
> +{
> +  return nullptr;
> +}
> +
> +MessagePort* AudioWorkletNode::Port() const
> +{

This cannot be null.
I know that this is just a initial patch, but let's do something that could be tested.

Mark the webidl attribute with [Throws] and here do:

MessagePort* AudioWorkerNode::Port(ErrorResult& aRv) const
{
  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
  return nullptr;
}

Comment 32

[Arnaud Bienner]

Comment on attachment 8982594 [details]
Bug 1458446 - Add AudioWorkletNode interface definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248566/diff/4-5/

Comment 33

[Arnaud Bienner]

Patched updated.
Karl, Andrea: do want to do a final review or we're all good and I can request checkin?
Meanwhile, I've triggered a try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae6db7ba5c291fd8ddb808ae8c0f0fea223be6a5

Comment 34

[Arnaud Bienner]

Try is green

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af8ef80f3326
Add AudioWorkletNode interface definitions. r=baku,karlt

Comment 36

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/af8ef80f3326

Comment 37

[Eric Shepherd [:sheppy]]

Documentation team note: AudioWorklet is currently not expected to ship enabled by default until sometime early in 2019. Given that and some things which are still in flux, we are holding off on documenting it until it’s closer to ready. See bug 1473176, “Document AudioWorklet”.