Add "AudioWorklet" definition

RESOLVED FIXED in mozilla63

Status

()

enhancement
P2
normal
Rank:
15
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {dev-doc-needed})

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 disabled)

Details

(URL)

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

11 months ago
Similar to bug 1466182, bug 1458446 and bug 1460907: add definitions for AudioWorklet interface.
Once this is done, we can add a "readonly attribute AudioWorklet audioWorklet;" to BaseAudioContext, behind Pref="dom.audioworklet.enabled"

"AudioWorklet" interface inherits from Worklet interface, which has already been defined, as part of bug 1290021.
But looking at comments there, I'm not sure what is missing in worklet's implementation.

I believe nothing if we just want the interfaces to be defined as part of this bug.
But probably some bits if we want to have working audio worklets (for example bug 1290021 comment 42 says worklet are not running in a separate thread).
Andrea, maybe you know more about worklet's current state?
Blocks: 1062849
Depends on: worklets-1
Flags: needinfo?(amarchesini)
Rank: 15
Priority: -- → P2
AudioWorklet is not exposed yet. Only Worklet interface is exposed. We should add it.
This new interface should be used by the 'audioWorklet' attribute in BaseAudioContext.

Note that we already have a Worklet interface, and, if that object is created with 'eAudioWorklet' type, it spawns a thread, and it runs JS using AudioWorkletGlobalScope. See:
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/worklet/Worklet.h#70

You want to change this too:
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/worklet/Worklet.cpp#511
In order to use AudioWorklet binding.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 3

11 months ago
Do you have an opinion about whether AudioWorklet should be a new C++ class inheriting Worklet, or implemented within Worklet class?

Initially, I was thinking about inheriting, but Karl pointed out that keeping everything in Worklet class is possible, and probably easier since AudioWorket interface doesn't need any new method/attribute.

I understood your previous comment as "inherit and reimplement WrapObject" but I suspect you actually meant "have a if in Worklet::WrapObject".

Just want to be sure before starting to write something :)
Flags: needinfo?(amarchesini)
> Initially, I was thinking about inheriting, but Karl pointed out that

I agree with Karl.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 5

11 months ago
OK thanks.

I have another question, sorry for the spam ;)

Implementing the new AudioWorklet interface is easy indeed.

But I tried to add an audioContext attribute on BaseAudioContext, and I'm not sure how this should be implemented: I should probably create a worklet, but worklet's constructor requires an nsIPrincipal object, and I'm not sure if this available on audioContext (nor what that corresponds to exactly).

Side note: I see AudioWorklet is currently created from the main window. Karl suggested this is the case because audioWorklet was initially defined in Window, before being moved to BaseAudioContext.
We should probably remove that code, now or later.
Flags: needinfo?(amarchesini)
> But I tried to add an audioContext attribute on BaseAudioContext, and I'm
> not sure how this should be implemented: I should probably create a worklet,
> but worklet's constructor requires an nsIPrincipal object, and I'm not sure
> if this available on audioContext (nor what that corresponds to exactly).

This part is easy too. AudioContext is a DOMEventTargetHelper. That means you can do:

nsCOMPtr<nsPIDOMWindowInner> window = audioContext->GetOwner();
if (NS_WARN_IF(!window)) { ... }
nsCOMPtr<nsIPrincipal> principal = nsGlobalWindowInner::Cast(window)->GetPrincipal();
if (NS_WARN_IF(!principal)) { ... }
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months ago
We should also update window.webidl to remove audioWorklet attribute there.
I suggest to do this in a followup patch though, to keep things clearer, unless you think otherwise.

Comment 9

11 months ago
mozreview-review
Comment on attachment 8988103 [details]
Bug 1470856 - Add AudioWorklet definition.

https://reviewboard.mozilla.org/r/253360/#review259982

::: dom/webidl/AudioWorklet.webidl:14
(Diff revision 1)
> + * Copyright © 2018 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> + [Exposed=Window, SecureContext, Pref="dom.audioworklet.enabled"]
> +    interface AudioWorklet : Worklet {

indentation.

::: dom/worklet/tests/test_audioWorklet.html:43
(Diff revision 1)
>  }
>  
>  // This function is called into an iframe.
>  function runTestInIframe() {
> -  audioWorklet.import("worklet_audioWorklet.js")
> +  ok(window.isSecureContext, "Test should run in secure context");
> +  var audioContext = new AudioContext();

Add these tests:

1. audioContext.audioWorklet should not be exposed for non-secure contexts
2. audioWorklet instanceof AudioWorklet
Attachment #8988103 - Flags: review?(amarchesini) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Assignee: nobody → arnaud.bienner

Comment 12

11 months ago
mozreview-review
Comment on attachment 8988103 [details]
Bug 1470856 - Add AudioWorklet definition.

https://reviewboard.mozilla.org/r/253360/#review260250

::: dom/media/webaudio/AudioContext.h:16
(Diff revision 2)
>  #include "MediaBufferDecoder.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/DOMEventTargetHelper.h"
>  #include "mozilla/MemoryReporting.h"
>  #include "mozilla/dom/TypedArray.h"
> +#include "mozilla/dom/Worklet.h"

Can mozilla::dom::Worklet be forward declared, please?

Many headers include this file, but few need Worklet.
I think we can depend on those that need Worklet to include mozilla/dom/Worklet.h.

As a precedence, nsGlobalWindowInner.h forward declares Worklet and
WindowBinding.cpp includes.

::: dom/worklet/tests/test_audioWorklet_insecureContext.html:4
(Diff revision 2)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for AudioWorklet</title>

Perhaps "Test for No AudioWorklet".

::: dom/worklet/tests/test_audioWorklet_insecureContext.html:23
(Diff revision 2)
> +  if ("audioWorklet" in audioContext) {
> +    ok(false, "AudioWorklet shouldn't be defined in AudioContext in a insecure context");
> +  } else {
> +    ok(true, "AudioWorklet shouldn't be defined in AudioContext in a insecure context");
> +  }

ok(!("audioWorklet" in audioContext),
     "AudioWorklet shouldn't be defined in AudioContext in a insecure context");

should work, I expect.
Attachment #8988103 - Flags: review?(karlt) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Blocks: 1471843
(Assignee)

Comment 15

11 months ago
As discussed on IRC with Andrea, the failing tests are unrelated, and I can't reproduce them locally, so requesting check-in.
Keywords: checkin-needed
I couldn't land your patch. Arnaud Bienner: Please set the issues opened by the reviewer as fixed by commit so review board allows to land them. Thank you.
Flags: needinfo?(arnaud.bienner)
(Assignee)

Comment 17

11 months ago
Sorry about that: issues marked as fixed.
Flags: needinfo?(arnaud.bienner)
Keywords: checkin-needed

Comment 18

11 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ce7c998c6346528481688f5e6e8943913e3bc9e8 -d d04c63b5a03b: rebasing 470536:ce7c998c6346 "Bug 1470856 - Add AudioWorklet definition. r=baku,karlt" (tip)
merging dom/media/webaudio/AudioContext.cpp
merging dom/worklet/Worklet.cpp
warning: conflicts while merging dom/worklet/Worklet.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Arnaud Bienner: Could you please have a look at the previous comment (Comment 18) ?
Flags: needinfo?(arnaud.bienner)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

11 months ago
Rebase + update binding calls: s/Binding/_Binding/
Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8c5fc68aa9cb76359a69426c0fa75dbf99fc4ef
Flags: needinfo?(arnaud.bienner)
(Assignee)

Comment 22

11 months ago
Try is green
Keywords: checkin-needed

Comment 23

11 months ago
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74e0bf980eae
Add AudioWorklet definition. r=baku,karlt
Keywords: checkin-needed

Comment 24

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74e0bf980eae
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1473176
You need to log in before you can comment on or make changes to this bug.