1466182 - Add AudioWorkletProcessor interface definitions

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement, P2)

Comment 1

[Arnaud Bienner]

Attachment: Bug 1466182 - Add AudioWorkletProcessor definitions.

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

Comment 2

[Arnaud Bienner]

About previous patch: I'm not sure how to test this interface.
Adding it to test_interfaces doesn't seem to work. I suspect this is because AudioWorkletProcessor isn't exposed in Window.
I suspect we should add a new test_worklet_interfaces.js (or test_audioworklet) in dom/worklet/tests, similarly to test_worker_interfaces.js for interfaces which are exposed only in Worker (e.g. WorkerLocation).
Not sure if this should be done now or later.

Comment 3

[Arnaud Bienner]

Looking at how Worker tests are implemented, I see they are similar to regular test_intefaces tests, but sent to the worker using postMessage so they can be executed in the correct context.
So unless we have implemented (message)port for AudioWorklet (on both node and processor side), we won't be able to add interface tests.

As soon as those are implemented, we should add those tests.

Comment 4

[Arnaud Bienner]

One last thing about the current patch: I couldn't put a Pref in the webidl, because AudioWorkletProcessor is not exposed to Window, so Pref isn't available.
I'm not sure this is really an issue, since AudioWorkletProcessor won't be acessible from Window, and all other interfaces using it indirectly are/will be guarded by a pref.

Comment 5

[Arnaud Bienner]

Karl: regarding our discussion yesterday, about whether or not it was correct to have window as parent object (for GetParentObject), I had a look to WorkerNavigator and WorkerLocation, both being interfaces exposed to Worker only, and they return nullptr for GetParentObject so this is probably what we should do here.

(Audio)WorkletGlobalScope, exposed only to AudioWorklet, also returns nullptr for GetParentObject.

What do you think?

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

https://reviewboard.mozilla.org/r/252606/#review259094

::: dom/bindings/DOMJSClass.h:109
(Diff revision 1)
>  static const uint32_t DedicatedWorkerGlobalScope = 1u << 2;
>  static const uint32_t SharedWorkerGlobalScope = 1u << 3;
>  static const uint32_t ServiceWorkerGlobalScope = 1u << 4;
>  static const uint32_t WorkerDebuggerGlobalScope = 1u << 5;
>  static const uint32_t WorkletGlobalScope = 1u << 6;
> +static const uint32_t AudioWorkletGlobalScope = 1u << 7;

I guess we need something added to IsNonExposedGlobal() also
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/bindings/BindingUtils.cpp#2958

::: dom/media/webaudio/AudioWorkletProcessor.h:28
(Diff revision 1)
> +  static already_AddRefed<AudioWorkletProcessor>
> +    Constructor(const GlobalObject& aGlobal,

Mozilla style is to align Constructor with static.

::: dom/media/webaudio/AudioWorkletProcessor.cpp:32
(Diff revision 1)
> +  nsCOMPtr<nsPIDOMWindowInner> window =
> +      do_QueryInterface(aGlobal.GetAsSupports());

AudioWorkletGlobalScope is not an nsPIDOMWindowInner.

GetParentObject could return nsIGlobalObject like MessagePort via DOMEventTargetHelper
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/workers/WorkerPrivate.cpp#5252
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/workers/WorkerPrivate.cpp#5252
and MessageChannel
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/messagechannel/MessageChannel.cpp#48

But I'll pass review onto baku, to see whether this is necessary.

::: dom/media/webaudio/AudioWorkletProcessor.cpp:34
(Diff revision 1)
> +  if (!window) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

MessageChannel just asserts nsIGlobalObject* aGlobal, so I guess that would be sufficient.

::: dom/media/webaudio/AudioWorkletProcessor.cpp:52
(Diff revision 1)
> +                                  JS::Handle<JSObject*> aGivenProto)
> +{
> +  return AudioWorkletProcessorBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +MessagePort* AudioWorkletProcessor::GetPort(ErrorResult& aRv) const

Start a new line at the start of "AudioWorkletProcessor".

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

https://reviewboard.mozilla.org/r/252606/#review259094

> AudioWorkletGlobalScope is not an nsPIDOMWindowInner.
> 
> GetParentObject could return nsIGlobalObject like MessagePort via DOMEventTargetHelper
> https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/workers/WorkerPrivate.cpp#5252
> https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/workers/WorkerPrivate.cpp#5252
> and MessageChannel
> https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/messagechannel/MessageChannel.cpp#48
> 
> But I'll pass review onto baku, to see whether this is necessary.

The second link was meant to be https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/base/StructuredCloneHolder.cpp#1162

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #5)
> Karl: regarding our discussion yesterday, about whether or not it was
> correct to have window as parent object (for GetParentObject), I had a look
> to WorkerNavigator and WorkerLocation, both being interfaces exposed to
> Worker only, and they return nullptr for GetParentObject so this is probably
> what we should do here.
> 
> (Audio)WorkletGlobalScope, exposed only to AudioWorklet, also returns
> nullptr for GetParentObject.

AudioWorkletGlobalScope is at the top of the parent hierarchy, so that may be why it returns nullptr.
I'm not familiar with the worker objects, and they seem a little different from MessageChannel/Port, so let's see what baku thinks.

Comment 9

[Andrea Marchesini [:baku]]

Inside the worklet global scope, you don't have a window, but you have a nsIGlobalObject which is the WorkerGlobalScope. In your case, this is also a AudioWorkletGlobalScope. In the WebIDL world, GetParentObject() is used to follow the chain of 'parent objects' until we have a nsIGlobalObject.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

https://reviewboard.mozilla.org/r/252606/#review259968

::: dom/media/webaudio/AudioWorkletProcessor.h:33
(Diff revision 1)
> +  static already_AddRefed<AudioWorkletProcessor>
> +    Constructor(const GlobalObject& aGlobal,
> +                const AudioWorkletNodeOptions& aOptions,
> +                ErrorResult& aRv);
> +
> +  nsPIDOMWindowInner* GetParentObject() const { return mParent; }

nsIGlobalObject GetParentObject() const { return mParent; }

::: dom/media/webaudio/AudioWorkletProcessor.h:41
(Diff revision 1)
> +                       JS::Handle<JSObject*> aGivenProto) override;
> +
> +  MessagePort* GetPort(ErrorResult& aRv) const;
> +
> +private:
> +  explicit AudioWorkletProcessor(nsPIDOMWindowInner* aParent);

In general, s/nsPIDOMWindowInner/nsIGlobalObject/g in this file.

::: dom/media/webaudio/AudioWorkletProcessor.h:43
(Diff revision 1)
> +  MessagePort* GetPort(ErrorResult& aRv) const;
> +
> +private:
> +  explicit AudioWorkletProcessor(nsPIDOMWindowInner* aParent);
> +  ~AudioWorkletProcessor() = default;
> +  nsCOMPtr<nsPIDOMWindowInner> mParent;

nsCOMPtr<nsIGlobalObject> mParent;

::: dom/media/webaudio/AudioWorkletProcessor.cpp:34
(Diff revision 1)
> +                                   const AudioWorkletNodeOptions& aOptions,
> +                                   ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsPIDOMWindowInner> window =
> +      do_QueryInterface(aGlobal.GetAsSupports());
> +  if (!window) {

Karlt is right here. What you want is:

nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());

and global cannot be null here:
MOZ_ASSERT(global);

Comment 11

[Arnaud Bienner]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

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

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

https://reviewboard.mozilla.org/r/252606/#review260086

::: dom/webidl/AudioWorkletProcessor.webidl:19
(Diff revision 2)
> + Constructor (optional AudioWorkletNodeOptions options)]
> +interface AudioWorkletProcessor {
> +    [Throws]
> +    readonly attribute MessagePort port;
> +};
> +

No extra line here.

Comment 13

[Arnaud Bienner]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

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

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

https://reviewboard.mozilla.org/r/252606/#review260228

Comment 15

[Arnaud Bienner]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66cbc5cefd92543f48f7cc01c9e946f51b8a7025

Comment 16

[Arnaud Bienner]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

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

Comment 17

[Arnaud Bienner]

Try failed. Compiler spotted a bug (not sure why this warning/error wasn't activated on my machine).
Fixed and push to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f4bb796652a32e5dc96b8ef829e1953ce3a1c37

Comment 18

[Arnaud Bienner]

New try job worked.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #17)
> (not sure why this warning/error wasn't activated on my machine).

Warnings are only errors (by default) on the build infrastructure, due to different compiler versions issuing different warnings.  I'll trigger autoland.

Comment 20

[Karl Tomlinson (:karlt)]

Actually, I can't until the outstanding issues are marked resolved, and I can't do that for Andrea's issues.
Can you resolve them, Arnaud?

Comment 21

[Arnaud Bienner]

Sorry about that: issues marked as fixed.

Comment 22

[Pulsebot]

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

Comment 23

[Eliza Balazs [:ebalazs_]]

Backed out changeset cfbcac62d08e (bug 1466182) for causing bustage builds/worker/workspace/build/src/dom/media/webaudio/AudioWorkletProcessor.cpp

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cfbcac62d08e6b552f56c858f314e45d15048b3a&selectedJob=185557797

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185557797&repo=autoland&lineNumber=21336

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6f13930cf9282fbc6f937b6993365197de0a85be

Comment 24

[Arnaud Bienner]

Comment on attachment 8987372 [details]
Bug 1466182 - Add AudioWorkletProcessor definitions.

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

Comment 25

[Arnaud Bienner]

Like for bug 1470856, IDL generated C++ classes have suffix "_Binding" instead of "Binding" so that's why the patch was broken.
Patch updated + push to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b32b2459a86127d37cb905e133c76c25dbbe27

Comment 26

[Arnaud Bienner]

Try is green

Comment 27

[Pulsebot]

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

Comment 28

[Andreea Pavel [:apavel]]

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