This sounds another bug 805333;) We are discussing some details in mails.

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

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.

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?

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!

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.

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.

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.

(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).

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().

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.

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

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

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!

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

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

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?

> 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 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 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. :)

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

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.

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

get windowUtils() is implemented here.

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

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

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.

> 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

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

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

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

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

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

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

> That is, is soundingAudioChannels a subset of activeAudioChannels? If so, "playingAndUnmuted" would be a better name, I think.

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

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.

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

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

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.

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.

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

(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.

> 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 on attachment 752637 [details] [diff] [review] part 0 - Agent + IPDL Moved to Bug 876631

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

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.

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.

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

link with AudioChannel meta.