853101 - Implement the new API for AudioChannels

Status: RESOLVED DUPLICATE of bug 1113086

(Core :: DOM: Core & HTML, defect, P2)

Description

[Andrea Marchesini [:baku]]

https://wiki.mozilla.org/WebAPI/AudioChannels

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

This sounds another bug 805333;)

We are discussing some details in mails.

Comment 2

[Marco Chen [:mchen]]

>> Turn down the volume of background audio rather than completely silence it when >> the notification channel is used 

About this requirement, I think it can be covered by interaction between AudioChannelService and it's clients. Please refer to Bug 823273.

That patch will let AudioChannelService send fading command not mute to channels with low priorities then notification. Any suggestion for this? Thanks.

Comment 3

[Marco Chen [:mchen]]

Refer to Bug 846092, the key tone would be needed to follow the ringer volume (ringer channel) but doesn't want to trigger AudioChannelService to pause lower channels.

So is it worth to add an Web API into AudioChannelManager for disabling media element joined AudioChannelService? So dialer app can set key tone to ringer channel type but not pause lower channels. Thanks.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

I think this one is more like a tracker bug and if it's possible we should list all related change/bugs first instead of discussing everything in the same bugs. Does this make sense?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: WIP

Ok.. this patch is getting huge and I need a feedback.
In this WIP I did the following things:

1. nsIDOMWindowUtils.idl has a set of new attributes/methods: audioPaused, audioVolume & C. I want to have them per window in order to use them in the browser API but maybe in ff too in the future.
I have choosen nsIDOMWindowUtils because it is commonly used in the Browser API and it's not directly exposed to content.

2. nsIDOMWindowUtils uses some private methods in nsPIDOMWindow.
This because the controller of the volume is per window. iframe, windows & C are nsPIDOMWindow - I'm not 100% sure about this.
Probably we have to implement something on-cascate approach for updating values in sub-windows...

3. any nsHTMLMediaElement is registered to nsPIDOMWindow and it updates its canPlay status any time it changes. I cannot use AudioChannelService because AudioChannelService is global and it doesn't have knowledge of windows (still thinking about ff - more than b2g).

4. nsPIDOMWindow takes care of refreshing the nsHTMLMediaElements with the new volume, paused/mused & C. It creates the list of soundingAudioChannels and activeAudioChannels.

5. treatNormalAsContent is propagated to AudioChannelService.

6. AudioChannelService is modified in order to use everywhere AudioChannelInternalType. This because the InternalType is calculated based on: new visibility, old visibility, new treatNormalAsContent, old treatNormalAsContent and I don't want to keep all these values in any method. So any registerType, unregisterType & C use the InternalType. IMO, the code is more readable.

7. When TreatNormalAsContent is called, the AudioChannelService changes the value in the array of mChannelCounters and it sends a notifications.

8. IPC is updated too.

This code compiles but I have to test it properly. In general I would like to know if you like the approach, if you have big big suggestions or different approach for the implementation.

Thank you!

Comment 6

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

Yes, lets just do API work here and do actual user-facing changes in other bugs since user facing changes are sure to need more discussion with UX.

Andrea, are you sure that windowutils is not accessible by content? Parts of it used to be.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Looks like we will soon have tef+ because bug 853454.
I am creating an gaia bug about removing workaround in bug 828283 depending on this one and take it, please tell me if you have any concern.

Other gaia UI bugs should be opened later according to the API changes in this bug, though it's not very clear now.

Comment 8

[Marco Chen [:mchen]]

Hi Andrea, 

I think I have no enough knowledge on giving feedback outside the AudioChannel and nsMediaElement so I would try to check these files only and please add someone to feedback others.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive] (PTO during 3/27~4/7) from comment #7)
> Looks like we will soon have tef+ because bug 853454.
> I am creating an gaia bug about removing workaround in bug 828283 depending
> on this one and take it, please tell me if you have any concern.
> 
> Other gaia UI bugs should be opened later according to the API changes in
> this bug, though it's not very clear now.

Macro told me this shouldn't be tef+ed and I also think so, so let's stay tuned until the patch here gets more mature and watch the status of bug 853454(not tef+ yet).

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 1 - AudioChannelService::TreatNormalAsContent

I'm splitting this patch in several patches but I'm not asking for a review until it's not completed tested by me. Feel free to give feedback :)

This first patch changes the AudioChannelService implementing the TreatNormalAsContent().

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow - WIP

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 3 - BrowserAPI - WIP

Comment 13

[Andrea Marchesini [:baku]]

Justin, can you quickly take a look of these 2 patches? I need a feedback.

My approach is to expose a couple of attributes/methods to the WindowUtils object. A better description is at comment 5. Thank you.

Comment 14

[Justin Lebar (not reading bugmail)]

> Andrea, are you sure that windowutils is not accessible by content? Parts of it used to be.

He's guarding everything with IsUniversalXPConnectCapable(), so we should be fine, I hope?

Comment 15

[Justin Lebar (not reading bugmail)]

Comment on attachment 728944 [details] [diff] [review]
part 3 - BrowserAPI - WIP

This is fine, but it seems more canonical to use an attribute on the iframe for this.  You could use a mutation observer to notice changes to the attribute.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 1 - AudioChannelService::TreatNormalAsContent

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 3 - BrowserAPI

Comment 19

[Andrea Marchesini [:baku]]

These 3 patches are almost ready. I'm doing the last tests and then I ask for reviews.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 3 - BrowserAPI

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 729513 [details] [diff] [review]
part 1 - AudioChannelService::TreatNormalAsContent

Justin, can you take a look of the IPC part?
And Marco can review the AudioChannelService logic. Thank you!

Comment 23

[Andrea Marchesini [:baku]]

A part the result of the review process, these 3 patches implement the new AudioChannel API. What is still missing is startBackgroundContentAudio().

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow

I uploaded the same patch twice. This is the right one.

Comment 25

[Marco Chen [:mchen]]

Comment on attachment 729513 [details] [diff] [review]
part 1 - AudioChannelService::TreatNormalAsContent

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

Is it possible that we just add mTreatNormalAsContent into argument of IPC functions from child to parent? so
  1. We can keep the logic corresponding to mTreatNormalAsContent in parent side only.
  2. Then there is no neccesary to expose "enum AudioChannelInternalType" to others which include AudioChannelCommon.h but never use that enum.
  3. For GetMutedInternal(...), attributes can be (AudioChannelType aType, uint64_t aChildID, bool aElementHidden, bool aTreatNormalAsContent).
     And GetMuted() can check whether elementHiden is really changed then just call GetMutedInternal(...). Thus new state is aElementHidden and old state is !aElementHidden.

::: dom/audiochannel/AudioChannelService.cpp
@@ +175,5 @@
> +  // agent any time a new one is registered.
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    SendAudioChannelChangedNotification();
> +    Notify();
> +  }

In the case of child process called AudioChannelServiceChild::TreatNormalAsContent(), this notification will not be fired. Then the IPC call to AudioChannelService::TreatNormalAsContentInternal(), there doesn't fire the notification too. So I suggest to move notification call to AudioChannelService::TreatNormalAsContentInternal().

@@ +199,5 @@
> +
> +    if (!mActiveContentChildIDs.Contains(aChildID)) {
> +      mActiveContentChildIDs.AppendElement(aChildID);
> +    }
> +  }

In case of audio playing in the background with content channel set by TreatNormalAsContent(true), do we need to take care the response of user calling TreatNormalAsContent(false). (content_hidden to normal_hidden) we need to clear the active ID too or this background audio can be played automatically again after TreatNormalAsContent(true).

::: dom/audiochannel/AudioChannelServiceChild.h
@@ +37,5 @@
>  
>    /**
>     * Return true if this type + this mozHidden should be muted.
>     */
> +  virtual bool GetMuted(AudioChannelAgent* aAgent, bool aMozHidden) MOZ_OVERRIDE;

The second argument in AudioChannelServiceChild.cpp is renamed to aElementHidden already. So could we correct it in this patch?

Comment 26

[Andrea Marchesini [:baku]]

> Is it possible that we just add mTreatNormalAsContent into argument of IPC
> functions from child to parent? so
>   1. We can keep the logic corresponding to mTreatNormalAsContent in parent
> side only.

Not really. Several reasons:

1. the mTreatNormalAsContent has to be stored by the child process too, otherwise GetMuted() will be called with wrong params.

2. The parent process don't have an Agent, but just counters, so it's hard to keep track of what happens in the child process

3. The internal types depend by 3 values: visibility + type + treatNormalAsContent. I don't like to have these 3 params in any method.

4. the logic in the child is just for generate the internal type value.

Then personally I like the idea that AudioChannelInternalType is used for communication parent<->children in IPC.


>   3. For GetMutedInternal(...), attributes can be (AudioChannelType aType,
> uint64_t aChildID, bool aElementHidden, bool aTreatNormalAsContent).
>      And GetMuted() can check whether elementHiden is really changed then
> just call GetMutedInternal(...). Thus new state is aElementHidden and old
> state is !aElementHidden.

Well, GetMutedInternal must know the previous state and the new one, mainly for 2 reasons:
- move values from a counter to another. It means that the previous state must be known.
- it changes the mActiveContentChildIDs/mActiveContentChildIDsFrozen when a mediaelement goes from visible to hidden or viceversa.

> In the case of child process called
> AudioChannelServiceChild::TreatNormalAsContent(), this notification will not
> be fired. Then the IPC call to
> AudioChannelService::TreatNormalAsContentInternal(), there doesn't fire the
> notification too. So I suggest to move notification call to
> AudioChannelService::TreatNormalAsContentInternal().

Right!

> > +    if (!mActiveContentChildIDs.Contains(aChildID)) {
> > +      mActiveContentChildIDs.AppendElement(aChildID);
> > +    }
> > +  }
> 
> In case of audio playing in the background with content channel set by
> TreatNormalAsContent(true), do we need to take care the response of user
> calling TreatNormalAsContent(false). (content_hidden to normal_hidden) we
> need to clear the active ID too or this background audio can be played
> automatically again after TreatNormalAsContent(true).

Right.

> The second argument in AudioChannelServiceChild.cpp is renamed to
> aElementHidden already. So could we correct it in this patch?

ok

Thanks!

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 1 - AudioChannelService::TreatNormalAsContent

Comment 28

[Marco Chen [:mchen]]

Comment on attachment 730091 [details] [diff] [review]
part 1 - AudioChannelService::TreatNormalAsContent

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

Hi Andrea,

It looks great.
And only one concern is about process priority.
Please put r=me after clarification of that one.

Thanks.

::: dom/audiochannel/AudioChannelService.cpp
@@ +188,5 @@
> +  // agent any time a new one is registered.
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    SendAudioChannelChangedNotification();
> +    Notify();
> +  }

In processPriorityManager, it will monitor the observer event of "audio-channel-agent-changed".
To consider the case as below,
  1. App is playing an audio with normal channel.
  2. Then audio is stopped due to switch to background.
  3. TreatNormalAsContent() is called then normal_hidden is became to content_hidden.

In rule of processPriorityManager, if a process contained channel with content_hidden then it will be "PROCESS_PRIORITY_BACKGROUND_PERCEIVABLE" not "PROCESS_PRIORITY_BACKGROUND" only. But according to the missing of "audio-channel-agent-changed", the priority of this process will not be updated.

@@ +270,5 @@
>  
>    // If the audio content channel is visible, let's remember this ChildID.
> +  if (aNewType == AUDIO_CHANNEL_INT_CONTENT &&
> +      (aOldType == AUDIO_CHANNEL_INT_NORMAL ||
> +       aOldType == AUDIO_CHANNEL_INT_CONTENT_HIDDEN)) {

Maybe we can add a comment for explaning the transition between normal to content is caused by treatNormalAsContent(true)

Comment 29

[Justin Lebar (not reading bugmail)]

Comment on attachment 729567 [details] [diff] [review]
part 3 - BrowserAPI

In general we don't change the browser API withoutadding tests.  Is it possible
to add mochitests for this?

Please wrap all code to 80 columns.

Do we have code in Gaia which uses these methods?  If possible I'd like to see
that code before we check this in.

>diff --git a/dom/browser-element/BrowserElementChildPreload.js b/dom/browser-element/BrowserElementChildPreload.js
>--- a/dom/browser-element/BrowserElementChildPreload.js
>+++ b/dom/browser-element/BrowserElementChildPreload.js

>@@ -495,16 +502,98 @@ BrowserElementChild.prototype = {
>+  _recvGetAudioPaused: function(data) {
>+    debug("Received getAudioPaused message: (" + data.json.id + ")");
>+
>+    let utils = content.QueryInterface(Ci.nsIInterfaceRequestor)
>+                       .getInterface(Ci.nsIDOMWindowUtils);

Would you mind writing |get windowUtils()|, like |get messageManager()| and
using it where appropriate, in new and existing code?

Bonus points if adding |get windowUtils()| and using it in existing code occurs
in a separate patch.  :)

>diff --git a/dom/browser-element/BrowserElementParent.jsm b/dom/browser-element/BrowserElementParent.jsm
>--- a/dom/browser-element/BrowserElementParent.jsm
>+++ b/dom/browser-element/BrowserElementParent.jsm

>+  defineMethod('audioActiveChannels', this._audioActiveChannels);
>+  defineMethod('audioSoundingChannels', this._audioSoundingChannels);
>+  defineMethod('treatNormalAsContentAudioChannel', this._treatNormalAsContentAudioChannel);

I think the first two should be getActiveAudioChannels and
getSoundingAudioChannels.

For the last one, at the very least it should be
treatNormalAsContentAudioChannels, but I think forceContentAudioChannels would
be a better name.  Or we could just use an attribute on the iframe, if you want
to support converting from normal --> content and also vice versa.

>@@ -491,16 +505,47 @@ BrowserElementParent.prototype = {
>   _stop: function() {
>     this._sendAsyncMsg('stop');
>   },
> 
>   _purgeHistory: function() {
>     return this._sendDOMRequest('purge-history');
>   },
> 
>+  _getAudioPaused: function(_value) {
>+    return this._sendDOMRequest('get-audio-paused');
>+  },
>+
>+  _setAudioPaused: function(_value) {
>+    return this._sendDOMRequest('set-audio-paused',
>+                                {value: _value});
>+  },
>+
>+  _getAudioVolume: function(_value) {
>+    return this._sendDOMRequest('get-audio-volume');
>+  },
>+
>+  _setAudioVolume: function(_value) {
>+    return this._sendDOMRequest('set-audio-volume',
>+                                {value: _value});
>+  },
>+
>+  _audioActiveChannels: function() {
>+    return this._sendDOMRequest('audio-active-channels');
>+  },
>+
>+  _audioSoundingChannels: function() {
>+    return this._sendDOMRequest('audio-sounding-channels');
>+  },

>+  _treatNormalAsContentAudioChannel: function(_enabled) {
>+    return this._sendDOMRequest('treat-normal-as-content-audio-channel',
>+                                {enabled: _enabled});
>+  },

Please add documentation as to what these functions return.  Here is as good a
place as any.  Some questions which need to be answered:

 * What is the range for audio volume: [0,1]?  0-100?  0-255?  (It should
   probably be made to be [0,1] if it's not.)

 * What is the difference between an "audio active channel" and an "audio
   sounding channel"?

 * audioActiveChannels and audioSoundingChannels return a list of strings.
   What does one of those strings represent?

 * What does "treat normal as content audio channel" do?

I only know the answer to one of these.  :)

Comment 30

[Justin Lebar (not reading bugmail)]

> For the last one, at the very least it should be
> treatNormalAsContentAudioChannels, but I think forceContentAudioChannels would
> be a better name.  Or we could just use an attribute on the iframe, if you want
> to support converting from normal --> content and also vice versa.

Oh, treatNormalAsContentAudioChannel takes a boolean argument.  I totally missed that.

It must have "set" in the name to indicate this.  So maybe setAudioChannelsAsContentType() would be an OK name.  Although I think "content-type audio channel" is a terrible name, and it should be called "elevated" or something.  Everything is "content" in our parlance; I don't get why "special audio channels" are "content".  But that ship has probably already sailed.

Comment 31

[Justin Lebar (not reading bugmail)]

Comment on attachment 729571 [details] [diff] [review]
part 2 - WindowUtils and GlobalWindow

Review of the IDL.

>diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
>--- a/dom/interfaces/base/nsIDOMWindowUtils.idl
>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl

Ah, documentation!  Could you please make sure that this answers all of the
questions I asked about the BEP code?  I'd also like comments in the BEP code
in addition to these, since that's the closest thing we have to a web-facing
IDL file.

>+   /**
>+    * With this it's possible to mute and pause all the MediaElement for
>+    * this window.
>+    */
>+   attribute boolean audioPaused;

s/MediaElement/MediaElements/.  Or you could say "<audio> and <video>
elements".

Which brings up a question: Does setting audioPaused pause <video> elements?
If so, it's probably not the right name...

Also, does setting audioPaused mute and pause all MediaElements in this window
and any subframes?

>+    /**
>+     * range: 0.0 to 1.0 - a custom volume
>+     */
>+    attribute float audioVolume;

I don't know what you mean by "- a custom value".

How are the audioVolume's of two nested frames merged?

>+    /**
>+     * list of active audio channels
>+     */
>+    readonly attribute nsIDOMDOMStringList activeAudioChannels;

What does "active" mean?

What does the string returned here correspond to?

Even though this is a sentence fragment, please start with a capital letter and
end it with a period.

>+    /**
>+     *
>+     * list of active and non muted audio channels
>+     */
>+    readonly attribute nsIDOMDOMStringList soundingAudioChannels;

Capital and period.

Remove the empty line above "list".

s/non muted/unmuted/.

(Interestingly "unmuted" is not in the OED.  But it's in Random House, so I
guess it's actually word...)

Now that I understand what it does, I don't think this is a great name.  But
I'm not sure yet what it should be called, because I don't know what an active
audio channel means.

Under what circumstances does a channel become muted?  For example, is a
channel muted if it's inside a window with audioVolume == 0?

Maybe in BrowserElementParent it would be better to merge activeAudioChannels
and soundingAudioChannels into one method which returns a list of objects:

  [{channel: "xyz", muted: true/false}]

Maybe we should also do that here; I care less about the internal interfaces
than the DOM interfaces, though.

>+    /*
>+     * when this attribute is set to true, any normal channel will be
>+     * 'converted' * to content channel. This fixes the problem of the
>+     * visibility.
>+     * Task of the system app is to set this boolean to true before changing
>+     * the visibility to false for the foreground app. Implementing this, we
>+     * can revert all the hacks we implemented for the visibility and normal
>+     * channels...
>+     */
>+    attribute boolean treatNormalAsContent;

Looks like a "*" crept in here.

Please put "audio channel" somewhere in this name and comment.

Please be more explicit what you mean about "the problem of visibility".

Please separate paragraphs with an empty line.

The first sentence in the second paragraph is problematic because Gecko doesn't
have a concept of the system app, and also because the system app doesn't
interact with this interface directly.  I like what I think you're getting at
with the second paragraph, but as written it's more of a bug comment than an
interface comment.

Comment 32

[Justin Lebar (not reading bugmail)]

To be clear, you don't need to answer these questions in bugzilla, but I think they do need to be answered in the IDL.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow

Comments + small bug fixes.
Mochitest is submitted in a separated patch.

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 3 - get windowUtils()

get windowUtils()  is implemented here.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 4 - Browser API

Comment 36

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 37

[Justin Lebar (not reading bugmail)]

> Comments + small bug fixes.

Did you forget to qref, or were the comment changes not to the IDL file?  It doesn't look much different.

Comment 38

[Andrea Marchesini [:baku]]

No... I didn't read your comments yet when I was working on the new patch

Comment 39

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

FWIW the advantage of keeping sohndingAudioChannels separate from activeAudioChannels is that it's easy to check if we have any sounding channels at all, which is likely the most common use case.

It also makes it easy to use. contains(channelname) for either list.

I'm OK either way though.

"active" in this context means "there's a media element using this channel which Is currently in the trying-to-play state. I.e. it's actively playing, or would play if we weren't force-pausing it".

Needs to be documented though.

Comment 40

[Andrea Marchesini [:baku]]

> Ah, documentation!  Could you please make sure that this answers all of the
> questions I asked about the BEP code?  I'd also like comments in the BEP code
> in addition to these, since that's the closest thing we have to a web-facing
> IDL file.

Sure... em... what is BEP?

> Which brings up a question: Does setting audioPaused pause <video> elements?
> If so, it's probably not the right name...

I proposed audioMuted, but then it's not really a mute... any better name?

> Also, does setting audioPaused mute and pause all MediaElements in this
> window
> and any subframes?

Right. We mute any subframes MediaElements.

> How are the audioVolume's of two nested frames merged?

The new volume is a multiplication of the values of any volume value of any parent window.
I wrote a better documentation.

> Under what circumstances does a channel become muted?  For example, is a
> channel muted if it's inside a window with audioVolume == 0?

Right. but it's not muted... it's paused. The doc has been updated.

> >+    attribute boolean treatNormalAsContent;
> 
> Looks like a "*" crept in here.
> 
> Please put "audio channel" somewhere in this name and comment.

treatNormalAudioChannelsAsContent

Comment 41

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow

I propose here a new patch with the comments fixed (except BEP :)

Comment 42

[Andrea Marchesini [:baku]]

Attachment: part 4 - Browser API

Comment 43

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 44

[Andrea Marchesini [:baku]]

I'm going to improve the mochitests so that they check sub-iframes too.

Comment 45

[Justin Lebar (not reading bugmail)]

> FWIW the advantage of keeping sohndingAudioChannels separate from activeAudioChannels is that it's 
> easy to check if we have any sounding channels at all, which is likely the most common use case.

What I was thinking is that you guys might want to add additional data to the list of channels.  Today the one bit is "muted or not".  But you can imagine other pieces of data, such as whether it's an <audio> or a <video>.

Anyway I don't feel strongly about this part of the API.

Comment 46

[Andrea Marchesini [:baku]]

Attachment: part 6 - setBackgroundContent - WIP

Comment 47

[Andrea Marchesini [:baku]]

> In rule of processPriorityManager, if a process contained channel with
> content_hidden then it will be "PROCESS_PRIORITY_BACKGROUND_PERCEIVABLE" not
> "PROCESS_PRIORITY_BACKGROUND" only. But according to the missing of
> "audio-channel-agent-changed", the priority of this process will not be
> updated.

We can emit audio-channel-agent-changed when SetTreatNormalAsContent() is called.
And maybe we can change the name of this event. What do you think?

Comment 48

[Justin Lebar (not reading bugmail)]

Comment on attachment 730091 [details] [diff] [review]
part 1 - AudioChannelService::TreatNormalAsContent

I'm sorry it took me so long to get to these reviews, Andrea.

There are some suggestions here that I think could improve the clarity and
conciseness of this code, but the main reason I'm sending this back is because
of the granularity issue discussed below.

>+bool
>+AudioChannelService::IsHiddenType(AudioChannelInternalType aType)
>+{
>+  switch (aType) {
>+    case AUDIO_CHANNEL_INT_NORMAL:
>+    case AUDIO_CHANNEL_INT_CONTENT:
>+    case AUDIO_CHANNEL_INT_NOTIFICATION:
>+    case AUDIO_CHANNEL_INT_ALARM:
>+    case AUDIO_CHANNEL_INT_TELEPHONY:
>+    case AUDIO_CHANNEL_INT_RINGER:
>+    case AUDIO_CHANNEL_INT_PUBLICNOTIFICATION:
>+      return false;
>+
>+    case AUDIO_CHANNEL_INT_NORMAL_HIDDEN:
>+    case AUDIO_CHANNEL_INT_CONTENT_HIDDEN:
>+    case AUDIO_CHANNEL_INT_NOTIFICATION_HIDDEN:
>+    case AUDIO_CHANNEL_INT_ALARM_HIDDEN:
>+    case AUDIO_CHANNEL_INT_TELEPHONY_HIDDEN:
>+    case AUDIO_CHANNEL_INT_RINGER_HIDDEN:
>+    case AUDIO_CHANNEL_INT_PUBLICNOTIFICATION_HIDDEN:
>+    default:
>+      return true;
>+  }
>+}

I'm sorry to nit like this, but this is a bad way to use an enum.  There are
other places where we hurt because AudioChannelInternalType is not a class; for
example, in the implementation of
AudioChannelService::SendAudioChannelChangedNotification.

I think what you want here is a class that combines an AudioChannelType with a
boolean for vanilla or HIDDEN.

You'd have to write your own ParamTraits implementation for this class so you
can pass it through IPC, but this is pretty simple.  See
ipc/glue/IPCMessageUtils.h.

>diff --git a/dom/audiochannel/AudioChannelService.h b/dom/audiochannel/AudioChannelService.h
>--- a/dom/audiochannel/AudioChannelService.h
>+++ b/dom/audiochannel/AudioChannelService.h

>@@ -44,16 +44,23 @@ public:
> 
>   /**
>    * Any  audio channel agent that stops playing should unregister itself to
>    * this service.
>    */
>   virtual void UnregisterAudioChannelAgent(AudioChannelAgent* aAgent);
> 
>   /**
>+   * If if type of this audio channel is 'normal', it will be treated as
>+   * content.
>+   */
>+  virtual void TreatNormalAsContent(AudioChannelAgent* aAgent,
>+                                    bool aEnabled);

We don't usually write comments in the future tense.  One more common way is to
use the imperative mood: 

  If this audio channel's type is 'normal', treat is as 'content'.  If the
  audio channel's type is not 'normal' when this method is called, do nothing,
  even if the audio channel later changes to the 'normal' type.

Notice the change from "it will be treated as content" (future passive
indicative) to "treat it as content" (present active imperative).

>+  /**
>+
>    * Return true if this type should be muted.
>    */

I don't think you meant to add this whitespace.

>   AudioChannelInternalType GetInternalType(AudioChannelType aType,
>-                                           bool aElementHidden);
>+                                           bool aElementHidden,
>+                                           bool aTreatNormalAsContent);

>+  bool IsHiddenType(AudioChannelInternalType aType);

These could be methods on the new AudioChannelInternalType class, if you decide
that's a reasonable design to try.

>+void
>+AudioChannelService::TreatNormalAsContentInternal(AudioChannelInternalType aNewType,
>+                                                  AudioChannelInternalType aOldType,
>+                                                  uint64_t aChildID)

This only has per-process granularity.

But we're exposing TreatNormalAsContent as a method on <iframe mozbrowser>, and
there can be multiple iframes all living in one process (e.g. as a result of
popup windows).

I think we want per-frame granularity here.  Not only would this be correct in
the face of popups, it would also give us the flexibility to run unrelated
browser tabs in the same process in order to save memory.

>+void
>+AudioChannelServiceChild::TreatNormalAsContent(AudioChannelAgent* aAgent,
>+                                               bool aEnabled)
>+{
>+  AudioChannelAgentData *data;
>+  if (!mAgents.Get(aAgent, &data) || !data ||
>+      data->mTreatNormalAsContent == aEnabled) {
>+    return;
>+  }
>+
>+  bool oldTreatNormalAsContent = data->mTreatNormalAsContent;
>+  AudioChannelService::TreatNormalAsContent(aAgent, aEnabled);
>+
>+  ContentChild *cc = ContentChild::GetSingleton();
>+  if (cc) {
>+    // Calculating the new and old type.

Again the imperative mood would be better here: "Calculate the new and old
types."

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>--- a/dom/ipc/ContentParent.h
>+++ b/dom/ipc/ContentParent.h

>@@ -360,24 +360,25 @@ private:
>                                  const uint32_t& aColNumber,
>                                  const uint32_t& aFlags,
>                                  const nsCString& aCategory);
> 
>     virtual bool RecvPrivateDocShellsExist(const bool& aExist);
> 
>     virtual bool RecvFirstIdle();
> 
>-    virtual bool RecvAudioChannelGetMuted(const AudioChannelType& aType,
>-                                          const bool& aElementHidden,
>-                                          const bool& aElementWasHidden,
>+    virtual bool RecvAudioChannelGetMuted(const AudioChannelInternalType& aNewType,
>+                                          const AudioChannelInternalType& aOldType,
>                                           bool* aValue);
> 
>-    virtual bool RecvAudioChannelRegisterType(const AudioChannelType& aType);
>-    virtual bool RecvAudioChannelUnregisterType(const AudioChannelType& aType,
>-                                                const bool& aElementHidden);
>+    virtual bool RecvAudioChannelTreatNormalAsContent(const AudioChannelInternalType& aNewType,
>+                                                      const AudioChannelInternalType& aOldType);
>+
>+    virtual bool RecvAudioChannelRegisterType(const AudioChannelInternalType& aType);
>+    virtual bool RecvAudioChannelUnregisterType(const AudioChannelInternalType& aType);

It seems to me that these methods would be better on a new IPDL interface
managed by TabParent.  Because the interface would be managed by TabParent,
you'd be able to get the right granularity.  And because it would be a new
interface, you wouldn't need to forward every message manually like you do now.

Comment 49

[Justin Lebar (not reading bugmail)]

>> Which brings up a question: Does setting audioPaused pause <video> elements?
>> If so, it's probably not the right name...
>
> I proposed audioMuted, but then it's not really a mute... any better name?

pauseMediaElements / mediaElementsPaused?

Comment 50

[Justin Lebar (not reading bugmail)]

Comment on attachment 731060 [details] [diff] [review]
part 4 - Browser API

Do we have code in Gaia which uses these methods?  If possible I'd like to see
that code before we check this in.

>diff --git a/dom/browser-element/BrowserElementParent.jsm b/dom/browser-element/BrowserElementParent.jsm
>--- a/dom/browser-element/BrowserElementParent.jsm
>+++ b/dom/browser-element/BrowserElementParent.jsm

>+  _getAudioPaused: function(_value) {
>+  _setAudioPaused: function(_value) {
>+  _getAudioVolume: function(_value) {
>+  _setAudioVolume: function(_value) {
>+  _getTreatNormalAudioChannelsAsContent: function() {
>+  _setTreatNormalAudioChannelsAsContent: function(_enabled) {

I think these would be better as attributes.  We can use a mutation observer to
notice changes to the attributes.

The advantage of using attributes is that reading them is easy (we don't have
to do a DOM request dance).  Additionally, you can setAttribute on the iframe
before the mozbrowser machinery loads.  (Right now, setAudioVolume will have
the same problem that setVisible has, that it's not defined until some time
after the mozbrowser is inserted into the DOM.)

>+  // getAudioActiveChannels().onsuccess(channels) returns an array
>+  // of the active channels used by any active media element.
>+  // An active media element is an element that is playing audio (but it
>+  // can be muted - read the AudioChannel policy).
>+  _getAudioActiveChannels: function() {
>+    return this._sendDOMRequest('get-audio-active-channels');
>+  },

Now that I know what you mean, I think "playing" would be a better name here.

I still don't know what things are returned by this method.  I recall that it
returns a list of strings, but I don't know how to convert from an audio
channel to a string.

>+  // getAudioSoundingChannels().onsucces(channels) returns an array
>+  // of the active channels that are not muted.
>+  _getAudioSoundingChannels: function() {
>+    return this._sendDOMRequest('get-audio-sounding-channels');
>+  },

If we're going to go with the two-method approach, we cannot call this
"sounding", which is not a word.  I suggested "unmuted"; does that work for
you?

I'm still not clear on what this means, though.  An active audio channel is
playing, but may be muted.  A sounding audio channel is not muted.  But is a
sounding audio channel necessarily playing?  That is, is soundingAudioChannels
a subset of activeAudioChannels?

I think this confusion strongly argues in favor of the API I suggested, where
we return a list of objects

> [{channel: "xyz", muted: true/false, playing: true/false, isVideo: true/false}]

I know Jonas disagrees about this API, but I feel it's kind of unfair to ask me
to r+ changes that I think are wrong to code that I nominally own.  Jonas, I
don't take it personally if you want to pull rank on me here, but then are you
willing to review these patches, or are you able to find someone else to do so?

Comment 51

[Justin Lebar (not reading bugmail)]

> I know Jonas disagrees about this API, but I feel it's kind of unfair to ask me
> to r+ changes that I think are wrong to code that I nominally own.  Jonas, I
> don't take it personally if you want to pull rank on me here, but then are you
> willing to review these patches, or are you able to find someone else to do so?

Sorry, this was way more prickly than was called for, particularly since Jonas said above that he didn't feel strongly about this particular API design choice.

Comment 52

[Justin Lebar (not reading bugmail)]

> That is, is soundingAudioChannels a subset of activeAudioChannels?

If so, "playingAndUnmuted" would be a better name, I think.

Comment 53

[Andrea Marchesini [:baku]]

> I know Jonas disagrees about this API, but I feel it's kind of unfair to ask
> me
> to r+ changes that I think are wrong to code that I nominally own.  Jonas, I
> don't take it personally if you want to pull rank on me here, but then are
> you
> willing to review these patches, or are you able to find someone else to do
> so?

Ok... let's do this. Let's freeze this code and let's talk face to face in order to find good solutions. I'm sure we can find a better API, or a better implementation.

Comment 54

[Andrea Marchesini [:baku]]

Attachment: part 0 - Agent + IPDL

This patch is huge. Sorry :/
It implements:

. AudioChannelAgent IPDL

. AudioChannelAgentChild - Created when nsIAudioChannelAgent is needed
When the AudioChannelAgentChild runs on the main process, it uses AudioChannelService directly. Otherwise it creates an AudioChannelAgentParent.

. AudioChannelAgentParent - this exists only when AudioChannelAgentChild is playing audio (StartPlaying - StopPlaying).

. AudioChannelServiceChild is gone.

. AudioChannelService uses just AudioChannelAgents.

Comment 55

[Andrea Marchesini [:baku]]

Next patches have to be updated. But I want to be sure that this first patch is good enough.

Comment 56

[Andrea Marchesini [:baku]]

Attachment: part 0 - Agent + IPDL

Please, read the comment for the previous version of this patch. I forgot some debug messages.

Comment 57

[Andrea Marchesini [:baku]]

Attachment: part 1 - AudioChannelService::TreatNormalAsContent

Comment 58

[Andrea Marchesini [:baku]]

Attachment: part 2 - WindowUtils and GlobalWindow

Comment 59

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 741364 [details] [diff] [review]
part 0 - Agent + IPDL

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

I think this looks good! r=me with this stuff fixed up.

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +25,5 @@
> +
> +AudioChannelGroupId::AudioChannelGroupId(nsIDOMWindow* aWindow)
> +: mType(Window)
> +{
> +  nsCOMPtr<nsPIDOMWindow> windowPrivate = do_QueryInterface(aWindow);

A static_cast should be all you need here.

@@ +29,5 @@
> +  nsCOMPtr<nsPIDOMWindow> windowPrivate = do_QueryInterface(aWindow);
> +  mId = windowPrivate->WindowID();
> +}
> +
> +AudioChannelGroupId::AudioChannelGroupId()

This one should be inlined.

@@ +36,5 @@
> +{
> +}
> +
> +bool
> +AudioChannelGroupId::operator==(const AudioChannelGroupId& id) const

This should be inlined too.

@@ +84,1 @@
>    mIsRegToService = true;

This is a lot less clear now since you're not really interacting with the service here any more. Maybe rename it? 'mStartedPlaying'?

::: dom/audiochannel/AudioChannelAgent.h
@@ +12,2 @@
>  #include "nsCOMPtr.h"
>  #include "nsWeakPtr.h"

You don't need these other includes anymore.

@@ +30,2 @@
>  
> +  // Helper

What does this mean?

@@ +33,5 @@
> +  {
> +    return static_cast<AudioChannelType>(mAudioChannelType);
> +  }
> +
> +  NS_IMETHOD GetAudioChannelType(int32_t* aAudioChannelType);

For all of these non-nsIFoo methods just use 'virtual nsresult' rather than NS_IMETHOD (it's clearer and it avoids the crazy calling convention weirdness on Windows).

@@ +53,3 @@
>    virtual ~AudioChannelAgent();
>  
> +  virtual bool StartPlayingReal() = 0;

Nit: s/Real/Internal/ ? Up to you, but it seems more intuitive to me.

::: dom/audiochannel/AudioChannelAgentChild.cpp
@@ +8,5 @@
> +#include "mozilla/dom/TabChild.h"
> +
> +using namespace mozilla::dom;
> +
> +NS_IMPL_ISUPPORTS1(AudioChannelAgentChild, nsIAudioChannelAgent)

You want the inherited macro here.

@@ +11,5 @@
> +
> +NS_IMPL_ISUPPORTS1(AudioChannelAgentChild, nsIAudioChannelAgent)
> +
> +AudioChannelAgentChild::AudioChannelAgentChild()
> +: mIPDL(false)

Nit: inline this and the destructor.

@@ +20,5 @@
> +{
> +}
> +
> +void
> +AudioChannelAgentChild::ActorDestroy(ActorDestroyReason aWhy)

Maybe only override this #ifdef DEBUG if it's just going to assert

@@ +70,5 @@
> +                    static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION) ==
> +                    AUDIO_CHANNEL_PUBLICNOTIFICATION,
> +                    "Enum of channel on nsIAudioChannelAgent.idl should be the same with AudioChannelCommon.h");
> +
> +  mGroupId = AudioChannelGroupId(aWindow);

Do you want to set this if you're going to bail out in a sec?

@@ +82,5 @@
> +  mAudioChannelType = aChannelType;
> +  mWindow = do_GetWeakReference(aWindow);
> +
> +  if (aUseWeakRef) {
> +    mWeakCallback = do_GetWeakReference(aCallback);

Seems like if this fails you should throw an error right?

@@ +84,5 @@
> +
> +  if (aUseWeakRef) {
> +    mWeakCallback = do_GetWeakReference(aCallback);
> +  } else {
> +    mCallback = aCallback;

What if it's null?

@@ +137,5 @@
> +AudioChannelAgentChild::SetVisibleReal()
> +{
> +  nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback();
> +  if (!callback) {
> +    return;

I'm curious if this is the right behavior. It seems like you'd still want to do stuff or whatever even if a weak callback has died...

@@ +146,5 @@
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    AudioChannelService* service = AudioChannelService::GetAudioChannelService();
> +    service->AgentChanged(this);
> +    canPlay = service->CanPlay(this);
> +  } else if (!SendSetVisible(mVisible, &canPlay)) {

This won't ever return false without crashing.

::: dom/audiochannel/AudioChannelAgentChild.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +/* Header file */

This is not needed :)

@@ +22,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  AudioChannelAgentChild();
> +
> +  virtual void NotifyAudioChannelStateChanged() MOZ_OVERRIDE;

You should use MOZ_OVERRIDE everywhere you can.

@@ +78,5 @@
> +
> +  bool RecvNotify(const bool& aCanPlay) MOZ_OVERRIDE;
> +
> +  // True when the IPDL Parent is active.
> +  bool mIPDL;

This needs a better name... 'mConnected'? 'mConnectedToParent'?

@@ +80,5 @@
> +
> +  // True when the IPDL Parent is active.
> +  bool mIPDL;
> +
> +  nsWeakPtr mWindow;

Nit: mWeakWindow is better imo

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +AudioChannelAgentParent::AudioChannelAgentParent(mozilla::dom::TabParent* aTabParent,

You shouldn't need the 'mozilla::dom::' here.

Can you inline this and the destructor?

@@ +23,5 @@
> +{
> +}
> +
> +void
> +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)

You should track this so that you don't call SendNotify/Send__delete__ if the child has crashed.

Also, wouldn't you want to unregister from the service, etc?

::: dom/audiochannel/AudioChannelCommon.h
@@ +35,5 @@
> +  AudioChannelGroupId();
> +  explicit AudioChannelGroupId(TabParent* aTabParent);
> +  explicit AudioChannelGroupId(nsIDOMWindow* aWindow);
> +
> +  bool operator==(const AudioChannelGroupId& aId) const;

You should comment where these things are implemented.

::: dom/audiochannel/AudioChannelService.h
@@ +16,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +class AudioChannelService : public nsISupports

Does this need to be refcounted anymore? At least I think you can just use the NS_INLINE_DECL_REFCOUNTING macro.

@@ +65,3 @@
>  
>  protected:
> +  class AudioChannelAgentData : public nsISupports {

Here too... Why refcounted? Why nsISupports?

::: dom/audiochannel/PAudioChannelAgent.ipdl
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +sync protocol PAudioChannelAgent

This protocol needs to be documented a bit. Explain the transition between states, etc.

::: dom/ipc/PContent.ipdl
@@ +431,5 @@
>      // Tell the parent that the child has gone idle for the first time
>      async FirstIdle();
>  
> +    // Return true if this child has active normal or content audio channels
> +    sync AudioChannelContentOrNormalIsActive()

This name needs some work...

::: dom/ipc/ProcessPriorityManager.cpp
@@ +482,5 @@
>    }
>  
> +  ContentChild* contentChild = ContentChild::GetSingleton();
> +
> +  bool active = false;

Nit: No need to initialize here, IPDL calls in the child will always set outparams (or crash safely).

::: dom/ipc/TabChild.cpp
@@ +1618,5 @@
> +TabChild::AllocPAudioChannelAgent(const AudioChannelType& type,
> +                                  const bool& visible,
> +                                  bool* canPlay)
> +{
> +    NS_RUNTIMEABORT("unused");

Nit: A better message here.

::: dom/ipc/TabChild.h
@@ +256,5 @@
>                                 const InfallibleTArray<nsString>& aStringParams,
>                                 nsIDialogParamBlock* aParams);
>  
> +    virtual PAudioChannelAgentChild*
> +    AllocPAudioChannelAgent(const AudioChannelType& type,

Nit: please add MOZ_OVERRIDE to this and the Dealloc method, and to all the other virtuals you added to TabParent.

@@ +260,5 @@
> +    AllocPAudioChannelAgent(const AudioChannelType& type,
> +                            const bool& visible,
> +                            bool* canPlay);
> +
> +    virtual bool DeallocPAudioChannelAgent(PAudioChannelAgentChild* actor);

Nit: C++ stuff here get 'aFoo' param names.

::: dom/ipc/TabParent.cpp
@@ +385,5 @@
> +    return agent.forget().get();
> +}
> +
> +bool
> +TabParent::RecvPAudioChannelAgentConstructor(PAudioChannelAgentParent* aActor,

Are there any security or permission checks that need to be run here? I don't know what our security model is here.

@@ +391,5 @@
> +                                             const bool& visible,
> +                                             bool* aCanPlay)
> +{
> +    nsRefPtr<AudioChannelAgentParent> agent =
> +        static_cast<AudioChannelAgentParent*>(aActor);

Nit: No need for the refptr here, you know it has at least one ref since you just gave it one.

::: dom/ipc/TabParent.h
@@ +219,5 @@
>  
>      static TabParent* GetFrom(nsFrameLoader* aFrameLoader);
>      static TabParent* GetFrom(nsIContent* aContent);
>  
> +    uint64_t Id() { return mId; }

Nit: make this a const method.

Comment 60

[Andrea Marchesini [:baku]]

Thanks!


> > +  nsCOMPtr<nsPIDOMWindow> windowPrivate = do_QueryInterface(aWindow);
> 
> A static_cast should be all you need here.

No.. It doesn't work.

> @@ +70,5 @@
> > +                    static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION) ==
> > +                    AUDIO_CHANNEL_PUBLICNOTIFICATION,
> > +                    "Enum of channel on nsIAudioChannelAgent.idl should be the same with AudioChannelCommon.h");
> > +
> > +  mGroupId = AudioChannelGroupId(aWindow);
> 
> Do you want to set this if you're going to bail out in a sec?

What do you mean?

> Also, wouldn't you want to unregister from the service, etc?

The idea is that:

1. the Child kills the Parent
2. the Parent unregisters it self when deleted

It should be enough. Is it?

> > +  class AudioChannelAgentData : public nsISupports {
> 
> Here too... Why refcounted? Why nsISupports?

Because of:
  nsTArray<nsRefPtr<AudioChannelAgentData> > mAgents;

I prefer to keep refcounted these objects. I removed nsISupports and now I use NS_INLINE_DECL_REFCOUNTING.

> > +    // Return true if this child has active normal or content audio channels
> > +    sync AudioChannelContentOrNormalIsActive()
> 
> This name needs some work...

hehe... any suggestions?

> > +    nsRefPtr<AudioChannelAgentParent> agent =
> > +        static_cast<AudioChannelAgentParent*>(aActor);
> 
> Nit: No need for the refptr here, you know it has at least one ref since you
> just gave it one.

Right. But I prefer to keep a reference. Maybe this object can be unref in one of the called method in the future.

Comment 61

[Andrea Marchesini [:baku]]

Attachment: part 0 - Agent + IPDL

I need a second round of reviews from Marco about the logic in AudioChannelService.

Comment 62

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #61)
> Created attachment 752637 [details] [diff] [review]
> part 0 - Agent + IPDL
> 
> I need a second round of reviews from Marco about the logic in
> AudioChannelService.

Is it possible to do a rebase then I can apply into tree for verification? Thanks.
By the way, I still recommend that this patch can be split out from this bug for just refactory the AudioChannlService.

Comment 63

[Andrea Marchesini [:baku]]

> Is it possible to do a rebase then I can apply into tree for verification?
> Thanks.

These patches are for b2g18 and they require the patch for Bug 862899.

> By the way, I still recommend that this patch can be split out from this bug
> for just refactory the AudioChannlService.

makes sense.

Comment 64

[Andrea Marchesini [:baku]]

Comment on attachment 752637 [details] [diff] [review]
part 0 - Agent + IPDL

Moved to Bug 876631

Comment 65

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Requesting this to be prioritized into 2.2, see bug 1015073 for user-facing issue.

Comment 66

[Chris Mills [:cmills]]

dev-doc-needed add; I'll be documenting this asap, as it's been in my to do list since June. ;-)

As an FYI, landing page is at https://developer.mozilla.org/en-US/docs/Web/API/AudioChannels_API

I'll ask for a doc review once I've finished documenting it.

Comment 67

[Chris Mills [:cmills]]

I’ve finished* documenting the Firefox OS-only AudioChannels API:

* Main landing page: https://developer.mozilla.org/en-US/docs/Web/API/AudioChannels_API
* Usage guide: https://developer.mozilla.org/en-US/docs/Web/API/AudioChannels_API/Using_the_AudioChannels_API
* AudioChannelManager interface: https://developer.mozilla.org/en-US/docs/Web/API/AudioChannelManager
* mozAudioChannelType property for audio/video: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement.mozAudioChannelType
* Demo on github: https://github.com/mdn/audio-channels-demo

I’d really appreciate a tech review, from anyone who has time.

* when I said finished, I appreciate that there are a few additions that aren’t yet implemented, like the Browser API parts of it, and the telephonySpeaker property of the manager. Let me know you notice anything else missing.

Comment 68

[Kai-Chih Hu [:khu]]

Bobby, is it in your area? Please follow up this topic.

Comment 69

[Bobby Chien]

link with AudioChannel meta.