Currently nsIAudioChannelManager was already removed in m-c tree and replaced by AudioChannelManager.webidl. But it is still created from navigator.getMozAudioChannelManager() and return as an "nsISupports". The return type of nsISupports caused the problem when AudioChannelManager needs to inherit another nsIXXX class. The compiler will have an error on link as below. http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1571 The error show "nsISupports is ambigulous.". So this bug trys to move AudioChannelManager from navigator to constructor of webidl.

Move the the issue from Description with new attribute together becuase that is caused by this new attruibute.

Purpose: Please refer to new attribute called volumeControlChannel in AudioChannelManager. https://wiki.mozilla.org/WebAPI/AudioChannels App can use this new attribute to define it's own default volume control channel. Ex: keypad.js in dialer app can set control channel to normal so volume key can be applied to normal not notification (default in sound_manager.js) Rough Idea: 1. AudioChannelManager a. will recieve the new volume control channel then sending to AudioChannelService. b. listen the visibility change then set volume control channel to notification when visibility is false. c. Permission check (ToDo) 2. AudioChannelService a. Notify event of default-volume-channel-changed when received request from AudioChannelManager. b. Listen the ipc:content-shutdown event then set volume control channel to notification when child from 2-a is dead. c. The request of AudioChannelManager in foreground can always change volume conrol channel and update mDefChannelChildID. d. The request in background needs to check whether it is the mDefChannelChildID. If not, it will be rejected. 3. shell.js a. To listen event of default-volume-channel-changed then send mozChromeEvent to system app. Others: I need to move AudioChannelManager from navigator to webidl contructor and the reason is on bug description. Maybe I also need to change it's name to mozAudioChannelManager in webidl.

Comment on attachment 731076 [details] [diff] [review] WIPv1 Review of attachment 731076 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/audiochannel/AudioChannelService.cpp @@ +287,5 @@ > } > > void > +AudioChannelService::SetDefaultVolumeControlChannel(AudioChannelType aType, > + bool aHidden) Indentation ::: dom/base/Navigator.cpp @@ +1567,5 @@ > mAudioChannelManager = new system::AudioChannelManager(); > mAudioChannelManager->Init(window); > } > > + //NS_ADDREF(*aAudioChannelManager = mAudioChannelManager); I cannot reproduce your problem linking this component... AudioChannelManager can be easily casted to nsISupports or to nsIDOMEventListener. ::: dom/bindings/Bindings.conf @@ +143,5 @@ > 'nativeType': 'mozilla::dom::battery::BatteryManager', > 'headerFile': 'BatteryManager.h' > }, > > +'AudioChannelManager': { This block should be merged with the previous one: line 97.

> I cannot reproduce your problem linking this component... > AudioChannelManager can be easily casted to nsISupports or to nsIDOMEventListener. The problem is not on casting to nsISupports but casting to nsISupports then calling AddRef(). In compile time, compiler can't know which nsISupports::AddRef()in nsIDOMEventListerne or nsDOMEventTargetHelper you want call.

(In reply to Marco Chen [:mchen] from comment #5) > > I cannot reproduce your problem linking this component... > > AudioChannelManager can be easily casted to nsISupports or to > > nsIDOMEventListener. Please refer to the diagram as below. AudioChannelManager inherited two copies of nsIsupports. So when you try to cast AudioChannelManager to nsISupports (in navigator.cpp), compiler will tell you an error about ambiguous on nsISupports. As I knew that normally I can use virtual inheritance for making nsISupports as one copy only but I am not sure I can do this way. May I know you suggestion? very thanks. nsISupports nsISupports ^ ^ | | nsIDOMEventListener nsIXXXXXX ^ ^ | | | nsDOMEventTargetHelper | ^ | | --------------------------------- | AudioChannelManager

If you don't use it in that navigator method, just do: nsresult NAvigator::TheMethod(nsISupports** retValue) { nsCOMPtr<nsIDOMEventListener> audioChannelManager = something->GetAudioChannelManager(); audioChannelManager.forget(retValue); }

Hi Jonas, This webidl added a new attribute called volumeControlChannel which is introduced at link as below. https://wiki.mozilla.org/WebAPI/AudioChannels

Can you propose a full patch before asking to jonas to review the webIDL ? So we ask him to review your code when everything is ready to land.

Hi Jonas, I am not sure who can help to review this so ask to you first. If you think who is suitable please help to transfer to him. Thanks. This patch is implementing the new attribute called volumeControlChannel. 1. Web content can set AudioChannel name (normal, content) to this attribute via mozAudioChannelManager for asking what default channel you want to adjust by HW volume key if there is no other audio stream alive. 2. AudioChannelManager will listen visibilitychanged event for setting default channel back to "default" if window is falling to background. Or set it to what user assigned if window is back to foreground. 3. AudioChannelService will record the childID for setting the default channel. a. If a child try to set default channel in the background and current recorded childID is not it then will skip this request. Because another foreground app already set it's default channel. b. If a child is crashed then AudioChannelService will clear default channel and recorded child ID. 4. AudioChannelService will notify default-volume-channel-changed event to shell.js then transferring to mozChromeEvent to System App. 5. The tricky way on Navigator::GetMozAudioChannelManager() is in order to avoid the dual inheritance of nsISupports. Please refer to comment 6 & 7. Thanks.

(In reply to Andrea Marchesini (:baku) from comment #9) > Can you propose a full patch before asking to jonas to review the webIDL ? > So we ask him to review your code when everything is ready to land. Hi Andrea, Thanks for your reminding. During your comment, I am writing the notes for patch in the same time.

Comment on attachment 752124 [details] [diff] [review] NewWebAPI - v1 Review of attachment 752124 [details] [diff] [review]: ----------------------------------------------------------------- I assume that the attribute is by default set to the empty string. Which would mean "use platform default"? If so, this looks good to me.

Comment on attachment 752128 [details] [diff] [review] Impl - v1 Review of attachment 752128 [details] [diff] [review]: ----------------------------------------------------------------- Andrea, can you take this review? I won't have time to get to it soon enough.

Sure, I can review this but, I was thinking that the idea was to implement this feature on top of the refactoring. Marco, any particular reason why this cannot happen?

(In reply to Andrea Marchesini (:baku) from comment #14) Hi Andrea, When I posted this patch your one for re-factoring of AudioChannelService is still on review process, in that time I am not sure what modification will be changed after you get review+. So I wrote this based on original code then I can do fully test without other pain. I also recommended you to split out patch of AudioChannelService from Bug 853101 in order to speed up this part and I can rebase my patch to new version. So my suggestion is that 1. The next version of patch will depend on re-factoring version. 2. Please give the first round of review based on this patch. Is this ok to you? Thanks.

Hi Andrea, According to the conclusion from mail (The Dependency Chain Made by Some Media Features for V1.2), the feature here will be based on current AudioChannelService (refactoring AudioChannelService will be postponed to V1.3). Could you help to review it or maybe you are out of bandwidth to do so then please suggest another one? Thanks.

Nominate it to koi+ and remove the dependency on 876631 to keep this feature moving on for v1.2

Comment on attachment 752128 [details] [diff] [review] Impl - v1 Review of attachment 752128 [details] [diff] [review]: ----------------------------------------------------------------- looks good. Can you apply these comments and send it for the final review? Thanks! ::: dom/audiochannel/AudioChannelService.h @@ +101,5 @@ > bool aElementHidden, bool aElementWasHidden); > > + /* Send the default-volume-channel-changed notification */ > + void SetDefaultVolumeControlChannelInternal( > + AudioChannelType aType, bool aHidden, uint64_t aChildID); same line. void SetDefaultVolumeControlChannelInternal(AudioChannelType aType, bool aHidden, uint64_t aChildID); ::: dom/base/Navigator.cpp @@ +1552,5 @@ > NS_IMETHODIMP > Navigator::GetMozAudioChannelManager(nsISupports** aAudioChannelManager) > { > *aAudioChannelManager = nullptr; > + nsCOMPtr<nsIDOMEventListener> manager = mAudioChannelManager; if mAudioChannelManager is a system::AudioChannelManager, you can just to do: nsCOMPtr<nsIDOMEventListener> manager = mAudioChannelManager. manager.forget(aAudioChannelManager); return NS_OK; @@ +1558,5 @@ > if (!mAudioChannelManager) { > nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow); > NS_ENSURE_TRUE(window, NS_OK); > mAudioChannelManager = new system::AudioChannelManager(); > + system::AudioChannelManager* temp = additional space. ::: dom/base/Navigator.h @@ +214,5 @@ > #ifdef MOZ_B2G_BT > nsCOMPtr<nsIDOMBluetoothManager> mBluetooth; > #endif > #ifdef MOZ_AUDIO_CHANNEL_MANAGER > + nsCOMPtr<nsIDOMEventListener> mAudioChannelManager; I think this should be kept as system::AudioChannelManager. ::: dom/ipc/ContentParent.cpp @@ +1424,5 @@ > + nsRefPtr<AudioChannelService> service = > + AudioChannelService::GetAudioChannelService(); > + if (service) { > + service->SetDefaultVolumeControlChannelInternal(aType, > + aHidden, mChildID); Same row. ::: dom/system/gonk/AudioChannelManager.cpp @@ +23,3 @@ > AudioChannelManager::AudioChannelManager() > + : mState(SWITCH_STATE_UNKNOWN), > + mVolumeChannel(AUDIO_CHANNEL_DEFAULT) what about: AudioChannelManager::AudioChannelManager() : mState(SWITCH_STATE_UNKNOWN) , mVolumeChannel(AUDIO_CHANNEL_DEFAULT) @@ +35,5 @@ > + > + nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow); > + NS_ENSURE_TRUE_VOID(window); > + nsCOMPtr<EventTarget> target = do_QueryInterface(window); > + NS_ENSURE_TRUE_VOID(target); nsCOMPtr<EventTarget> target = do_QueryInterface(GetOwner()); @@ +49,5 @@ > + nsPIDOMWindow* window = aWindow->IsOuterWindow() ? > + aWindow->GetCurrentInnerWindow() : aWindow; > + BindToOwner(window); > + > + mWindow = do_GetWeakReference(window); Why do you need to keep a WeakReference? is GetOwner() not enough? @@ +92,5 @@ > + // Only normal channel doesn't need permission. > + if (aChannel.EqualsASCII("normal")) { > + mVolumeChannel = AUDIO_CHANNEL_NORMAL; > + } else { > + nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow); GetOwner() @@ +153,5 @@ > + > +void > +AudioChannelManager::NotifyVolumeControlChannelChanged() > +{ > + nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow); GetOwner() @@ +170,5 @@ > + } > +} > + > +NS_IMETHODIMP > +AudioChannelManager::HandleEvent( same row ::: dom/system/gonk/AudioChannelManager.h @@ +63,5 @@ > > private: > hal::SwitchState mState; > + AudioChannelType mVolumeChannel; > + nsWeakPtr mWindow; can you get rid of this?

...and sorry for the delay!

I am working on Bug 823273 now. And will address Andrea's comment next week. Thanks for the review.

Hi Andrea, Please help to review this version addressed your comment and thanks.

Hi Alive, May I know that you have bandwidth to modify the sound_manager.js in Gaia for this change or not? If you have no bandwidth then I can try to do it then ask review from you. Thanks.

Comment on attachment 798793 [details] [diff] [review] Impl - v2 Review of attachment 798793 [details] [diff] [review]: ----------------------------------------------------------------- r=me and apply the comments. ::: dom/ipc/ContentParent.cpp @@ +1609,5 @@ > +{ > + nsRefPtr<AudioChannelService> service = > + AudioChannelService::GetAudioChannelService(); > + if (service) { > + service->SetDefaultVolumeControlChannelInternal(aType, This should stay on the same row. ::: dom/system/gonk/AudioChannelManager.cpp @@ +16,5 @@ > namespace mozilla { > namespace dom { > namespace system { > > +NS_IMPL_QUERY_INTERFACE_INHERITED1(AudioChannelManager, nsDOMEventTargetHelper, nsIDOMEventListener) 80 chars per line. @@ +74,5 @@ > +bool > +AudioChannelManager::SetVolumeControlChannel(const nsAString& aChannel) > +{ > + if (aChannel.EqualsASCII("publicnotification")) { > + return false; what is this true/false ? @@ +105,5 @@ > + } else if (aChannel.EqualsASCII("telephony")) { > + mVolumeChannel = AUDIO_CHANNEL_TELEPHONY; > + } else if (aChannel.EqualsASCII("ringer")) { > + mVolumeChannel = AUDIO_CHANNEL_RINGER; > + } else { some error...? could it be? } @@ +144,5 @@ > + nsCOMPtr<nsIDocShell> docshell = do_GetInterface(GetOwner()); > + NS_ENSURE_TRUE_VOID(docshell); > + > + bool isActive = false; > + docshell->GetIsActive(&isActive); Whay GetIsActive and not GetVisibility? If you are checking for the visibility, maybe you can use: GetVisibility from the window. ::: dom/system/gonk/AudioChannelManager.h @@ +18,5 @@ > > namespace dom { > namespace system { > > +class AudioChannelManager MOZ_FINAL why did you change he indentation here? @@ +54,5 @@ > } > > + bool SetVolumeControlChannel(const nsAString& aChannel); > + > + bool GetVolumeControlChannel(nsAString & aChannel); nsAString& aChannel

(In reply to Andrea Marchesini (:baku) from comment #23) > Comment on attachment 798793 [details] [diff] [review] > Impl - v2 > ::: dom/ipc/ContentParent.cpp > > @@ +74,5 @@ > > +bool > > +AudioChannelManager::SetVolumeControlChannel(const nsAString& aChannel) > > +{ > > + if (aChannel.EqualsASCII("publicnotification")) { > > + return false; > > what is this true/false ? > public notification is reserved to be the one which no body can modify it's volume level. So if any partner asked camera shutter sound can't be muted or lower volume level then we can put it as public notification. So it should not be assigned as default channel then be modified by volume key. > @@ +105,5 @@ > > + } else if (aChannel.EqualsASCII("telephony")) { > > + mVolumeChannel = AUDIO_CHANNEL_TELEPHONY; > > + } else if (aChannel.EqualsASCII("ringer")) { > > + mVolumeChannel = AUDIO_CHANNEL_RINGER; > > + } > > else { > some error...? could it be? > } > No, because except these strings, others will be broken in permission check. So after permission check, only these strings will go to here. > ::: dom/system/gonk/AudioChannelManager.h > @@ +18,5 @@ > > > > namespace dom { > > namespace system { > > > > +class AudioChannelManager MOZ_FINAL > > why did you change he indentation here? > Follow the indentation as the same to AudioChannelService.h ----------------- Others will be addressed in next version. And thanks for your help.

Introduction of this new Web API. The current volume control policy a. In lock screen or home screen, volume keys will control "notification + ringer". b. In others apps, it will be "normal + content". c. If there is any type of audio channels in playing, then highest priority channel will be controlled. What issue this Web API want to fix? a. No matter what is the default control channel, if it is not expected by a certain app then it can use this API to change it. What rule of this Web API? a. You need to declare audio-channel-XXX permission in manifest then you can set it as default. b. Only foreground app can set default channel. c. Default channel set by app will be cleared when it falls to background. d. When this app is killed by any reason. What I suggested? a. Set global default channel is "notification + ringer". b. Ask specific apps to explicitly assign their default channel to "normal + content" by this API. (ex: music, video) c. For dial app, it can set default channel to "normal + content" when keypad page is shown or still be "notification + ringer". (ex: call history and contacts) What gaia - sound_manager.js should follow? a. Listen mozChromEvent for default channel change from this new Web API. b. When pressing volume keys and no active audio is running, check the default channel above. c. If default channel change from this new Web API is "unknow", it means clear this explicitly change and apply the one set as default by sound_manager.js.

Let me figure out what's the default strategy for current gaia.. Maybe we need to add this to certain key app like music.

(In reply to Andrea Marchesini (:baku) from comment #23) > Comment on attachment 798793 [details] [diff] [review] > Impl - v2 > > Review of attachment 798793 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me and apply the comments. > > ::: dom/ipc/ContentParent.cpp > @@ +1609,5 @@ > > +{ > > + nsRefPtr<AudioChannelService> service = > > + AudioChannelService::GetAudioChannelService(); > > + if (service) { > > + service->SetDefaultVolumeControlChannelInternal(aType, > > This should stay on the same row. If we put it at the same line then chars will be more then 80. (82 chars)

(In reply to Andrea Marchesini (:baku) from comment #23) > Comment on attachment 798793 [details] [diff] [review] > Impl - v2 > > Review of attachment 798793 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +144,5 @@ > > + nsCOMPtr<nsIDocShell> docshell = do_GetInterface(GetOwner()); > > + NS_ENSURE_TRUE_VOID(docshell); > > + > > + bool isActive = false; > > + docshell->GetIsActive(&isActive); > > Whay GetIsActive and not GetVisibility? > If you are checking for the visibility, maybe you can use: GetVisibility > from the window. > Refer to the link as below, it used GetIsActive too. http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#107 May I know the reason for choosing the other one? Thanks.

In this new patch, I just addressed two nits as below. > +NS_IMPL_QUERY_INTERFACE_INHERITED1(AudioChannelManager, nsDOMEventTargetHelper, nsIDOMEventListener) > + bool GetVolumeControlChannel(nsAString & aChannel); For others, I replied on comment 24/27/28. According to there are many review comments not got your agree yet so I asked the review again. Thanks.

Comment on attachment 799297 [details] [diff] [review] 798793: Impl - v3 Review of attachment 799297 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but can you add to RegisterAudioChannelAgent() and other methods in AudioChannelService|Child: MOZ_ASSERT(aType != AUDIO_CHANNEL_DEFAULT); Makes sense? ::: dom/audiochannel/AudioChannelService.cpp @@ +323,5 @@ > + > + // If this child is in the background and mDefChannelChildID is set to > + // others then it means other child in the foreground already set it's > + // own default channel already. > + if (aHidden == false && mDefChannelChildID != aChildID) { if (!aHidden && ... @@ +556,5 @@ > Notify(); > + > + if (mDefChannelChildID == childID) { > + SetDefaultVolumeControlChannelInternal(AUDIO_CHANNEL_DEFAULT, > + false, false, childID)

https://tbpl.mozilla.org/?tree=Try&rev=276727b5ca45 All green

Carry the super reviewer name.

1. Address the three suggestions on comment 30. 2. Carry the reviewer name.

https://hg.mozilla.org/integration/b2g-inbound/rev/58cbc13f9f21 https://hg.mozilla.org/integration/b2g-inbound/rev/4cfaef826ee6

https://hg.mozilla.org/mozilla-central/rev/58cbc13f9f21 https://hg.mozilla.org/mozilla-central/rev/4cfaef826ee6