Add API to BrowserElement to mute and set volume at the browser level

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

({dev-doc-complete})

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Currently it's possible to access individual audio channels, but that doesn't allow for a mute all feature like Firefox just added.  This is because some audio channels can be started after a mute command happened.  

To support this a browser level mute/unmute and volume control is needed.
Attachment #8639992 - Flags: review?(fabrice)
Posted patch Tests! (obsolete) — Splinter Review
Attachment #8639994 - Flags: review?(fabrice)
Brian, this volume change/muting is global for the whole browser, right? If that is the case, I'm not sure that putting this api on every mozbrowser iframe is needed. Instead we could manage that from shell.js.
It's per iframe, so you can mute a noisy tab for example without muting other tabs.
Blocks: graphene
Comment on attachment 8639992 [details] [diff] [review]
Add API to BrowserElement to mute and set volume

Review of attachment 8639992 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but we need a DOM peer to sign off the webidl change.

Jonas, can you take a look at the webidl ?
Attachment #8639992 - Flags: review?(jonas)
Attachment #8639992 - Flags: review?(fabrice)
Attachment #8639992 - Flags: review+
Comment on attachment 8639994 [details] [diff] [review]
Tests!

Review of attachment 8639994 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

::: dom/browser-element/mochitest/browserElement_AudioChannel.js
@@ +45,5 @@
>        }
>      })
>  
>      .then(function() {
>        return new Promise(function(r, rr) {

whoever came up with these pararemeters names deserves... something bad

@@ +47,5 @@
>  
>      .then(function() {
>        return new Promise(function(r, rr) {
> +        iframe.setAudioMuted(true);
> +        iframe.getAudioMuted().onsuccess = function(e) {

note that DOMRequest are then-able so you can switch to iframe.getAudioMuted().then(...) if you prefer. Up to you.

@@ +66,5 @@
> +    })
> +
> +    .then(function() {
> +      return new Promise(function(r, rr) {
> +        iframe.setAudioVolume(0);

do we end up in muted state when turning the volume down to 0? if so we should test that.
Attachment #8639994 - Flags: review?(fabrice) → review+
> do we end up in muted state when turning the volume down to 0? if so we should test that.

Nope there are 2 properties so that the volume can be preserved.
So leaving as is.  

I'll add an alternate of Ehsan since he's doing a lot of sound indicator stuff for the webidl.
Comment on attachment 8639992 [details] [diff] [review]
Add API to BrowserElement to mute and set volume

Review of attachment 8639992 [details] [diff] [review]:
-----------------------------------------------------------------

Swapping to Ehsan because I chatted with him about it already
Attachment #8639992 - Flags: review?(jonas) → review?(ehsan)
Comment on attachment 8639992 [details] [diff] [review]
Add API to BrowserElement to mute and set volume

Review of attachment 8639992 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/BrowserElementAudioChannel.webidl
@@ +35,5 @@
>     CheckPermissions="browser"]
>    readonly attribute sequence<BrowserElementAudioChannel> allowedAudioChannels;
> +
> +  /**
> +   * Mute all current and future MediaElements in this browser.

This comment is not correct, since this is not limited to media elements, and it also works with anything else that can play audio.

@@ +40,5 @@
> +   */
> +  [Throws,
> +   Pref="dom.mozBrowserFramesEnabled",
> +   CheckAnyPermissions="browser"]
> +  void setAudioMuted(boolean audioMuted);

Instead of this, please add two methods: mute() and unmute().

@@ +48,5 @@
> +   */
> +  [Throws,
> +   Pref="dom.mozBrowserFramesEnabled",
> +   CheckAnyPermissions="browser"]
> +  DOMRequest getAudioMuted();

FWIW, for <xul:browser> we just maintain a boolean flag in the parent process <https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml?from=browser.xml&case=true#687> and send async messages to the child to mute/unmute from the respective methods.  You may want to do the same if returning a DOMRequest is too much of a pain.  Up to you.

@@ +65,5 @@
> +   */
> +  [Throws,
> +   Pref="dom.mozBrowserFramesEnabled",
> +   CheckAnyPermissions="browser"]
> +  DOMRequest getAudioVolume();

Ditto.
Attachment #8639992 - Flags: review?(ehsan) → review-
Whiteboard: enable
Whiteboard: enable
DOMRequests don't seem to be thenable btw, switching back to onsuccess:
https://developer.mozilla.org/en-US/docs/Web/API/DOMRequest
ah ok :)
Updated with review comments. Didn't get rid of the DOM requests since it was already done this way, it matches some other conventions, and I think it might be slightly more sane to get the value from the child browser. For example if we introduce a mute by default it may avoid a bug.
Attachment #8639992 - Attachment is obsolete: true
Attachment #8641267 - Flags: review?(ehsan)
Posted patch Tests! v2.Splinter Review
Carrying forward r+ on tests. Used then instead of onsuccess and updated to new API.
Attachment #8639994 - Attachment is obsolete: true
Attachment #8641269 - Flags: review+
Comment on attachment 8641267 [details] [diff] [review]
Add API to BrowserElement to mute and set volume. v2

Review of attachment 8641267 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the WebIDL changes.
Attachment #8641267 - Flags: review?(ehsan) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2cceda7b2fa2e3b9323d8f88db0761aaec8ee1
changeset:  8e2cceda7b2fa2e3b9323d8f88db0761aaec8ee1
user:       Brian R. Bondy <netzen@gmail.com>
date:       Fri Jul 31 13:21:18 2015 -0400
description:
Bug 1188487 - Add API to BrowserElement to mute and set volume. r=fabrice

It's already possible to get mute/volume at the audio channel level, but
this adds it at the iframe level so that audio channels can be created
and destroyed and the setting will be preserved.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/11cdfe4196641ef4bb9db1ede43a58f81760ce84
changeset:  11cdfe4196641ef4bb9db1ede43a58f81760ce84
user:       Brian R. Bondy <netzen@gmail.com>
date:       Fri Jul 31 13:21:22 2015 -0400
description:
Bug 1188487 - BrowserElement webidl changes for muting and setting volume. r=ehsan

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/b7224f54a957b471a3c4ff7e550e16442793f663
changeset:  b7224f54a957b471a3c4ff7e550e16442793f663
user:       Brian R. Bondy <netzen@gmail.com>
date:       Fri Jul 31 13:21:25 2015 -0400
description:
Bug 1188487 - Tests for BrowserElement mute / set volume. r=fabrice
Given that Baku has written most of the current audio channel handling in Gecko, he should probably be involved here.
You need to log in before you can comment on or make changes to this bug.