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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Brian R. Bondy [:bbondy]]

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.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Add API to BrowserElement to mute and set volume

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Tests!

Comment 3

[[:fabrice] Fabrice Desré]

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.

Comment 4

[Brian R. Bondy [:bbondy]]

It's per iframe, so you can mute a noisy tab for example without muting other tabs.

Comment 5

[[:fabrice] Fabrice Desré]

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 ?

Comment 6

[[:fabrice] Fabrice Desré]

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.

Comment 7

[Brian R. Bondy [:bbondy]]

> 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 8

[Brian R. Bondy [:bbondy]]

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

Comment 9

[(no longer active)]

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.

Comment 10

[Brian R. Bondy [:bbondy]]

DOMRequests don't seem to be thenable btw, switching back to onsuccess:
https://developer.mozilla.org/en-US/docs/Web/API/DOMRequest

Comment 11

[[:fabrice] Fabrice Desré]

The doc is not up to date:
https://mxr.mozilla.org/mozilla-central/source/dom/webidl/DOMRequest.webidl?force=1#24

Comment 12

[Brian R. Bondy [:bbondy]]

ah ok :)

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Add API to BrowserElement to mute and set volume. v2

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.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Tests! v2.

Carrying forward r+ on tests. Used then instead of onsuccess and updated to new API.

Comment 15

[(no longer active)]

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.

Comment 16

[Brian R. Bondy [:bbondy]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aca47c8e7f91

Comment 17

[Brian R. Bondy [:bbondy]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2cceda7b2fa2e3b9323d8f88db0761aaec8ee1
changeset:  8e2cceda7b2fa2e3b9323d8f88db0761aaec8ee1
user:       Brian R. Bondy <[e-mail]>
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 <[e-mail]>
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 <[e-mail]>
date:       Fri Jul 31 13:21:25 2015 -0400
description:
Bug 1188487 - Tests for BrowserElement mute / set volume. r=fabrice

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Given that Baku has written most of the current audio channel handling in Gecko, he should probably be involved here.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e2cceda7b2f
https://hg.mozilla.org/mozilla-central/rev/11cdfe419664
https://hg.mozilla.org/mozilla-central/rev/b7224f54a957

Comment 20

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/getMuted
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/getVolume
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/setVolume
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/mute
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/unmute