Alastor Wu [:alwu] Assignee
	Description • 9 years ago

Now, we are doing the audio channel refactor, and it will change the control ability to the audio channel from gecko to gaia. You can see more details in Bug1113086.

In the new audio channel design, every browser element has a BrowserElementAudioChannel. We could use it to manage the audio channels. Before we do that, we need to connect the BrowserElementAudioChannel with the AudioChannelAgent. The way to achieve that is to tell AudioChannelAgent who is your owner, then the AudioChannelAgent could send some events to the specific BrowserElementAudioChannel.

In normal situation, we use the media element to playback the media. The media element would tell the agent who is its owner. But, when we use the telephony, we will not to create a media element or something else. Because of that, we can't use the BrowserElementAudioChannel to manage the telephony sound.

Therefore, we want to create an AudioChannelAgent in the telephony object. The telephony object know who is its owner, then it could tell the AudioChannelAgent too.

Here is my rough idea, 
(1) Add a function in MozTelephonyAPI, "EnableTelephonyAudioAgent()". It could be used to create an AudioChannelAgent which is belonged to the caller.

(2) Register the AudioChannelAgent when the telephony sound starts playback. Unregister the AudioChannelAgent when the telephony sound stops playback. It would depend on the call state.

	Alastor Wu [:alwu] Assignee
	Comment 1 • 9 years ago

This patch implement the concept in the description.

(1) New function in MozTelephonyAPI, "EnableTelephonyAudioAgent()"
(2) Add the audio agent in the telephony object
(3) Decide whether to register or unregister the agent by checking the call state.

Hi, Aknow & Baku,
Could you give me some suggestions?
Very appreciate :)

	Andrea Marchesini [:baku]
	Comment 3 • 9 years ago

Comment on attachment 8559744 [details] [diff] [review]
Create new function and audio agent in telephony object

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

::: dom/browser-element/BrowserElementAudioChannel.cpp
@@ +15,5 @@
>  #include "nsIObserverService.h"
>  #include "nsISupportsPrimitives.h"
>  #include "nsITabParent.h"
>  #include "nsPIDOMWindow.h"
> +#include "mozilla/dom/AudioChannelBinding.h"

do we need this?

::: dom/telephony/Telephony.cpp
@@ +527,5 @@
> +void
> +Telephony::EnableTelephonyAudioAgent(ErrorResult& aRv)
> +{
> +  if (!mTelephonyAudioAgent) {
> +    mTelephonyAudioAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");

MOZ_ASSERT(mTelephonyAudioAgent);

@@ +529,5 @@
> +{
> +  if (!mTelephonyAudioAgent) {
> +    mTelephonyAudioAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
> +    mTelephonyAudioAgent->Init(GetParentObject(),
> +                              (int32_t)AudioChannel::Telephony, nullptr);

no callbacks? It means that we cannot mute/unmute/change the volume for the telephony channel?

@@ +533,5 @@
> +                              (int32_t)AudioChannel::Telephony, nullptr);
> +  }
> +}
> +
> +void

nsresult

@@ +543,5 @@
> +
> +  Nullable<OwningTelephonyCallOrTelephonyCallGroup> isTelephonyAudioActive;
> +  GetActive(isTelephonyAudioActive);
> +  if (isTelephonyAudioActive.IsNull()) {
> +    mTelephonyAudioAgent->StopPlaying();

nsresult rv = mTelephonyAudioChannel->StopPlaying();
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +547,5 @@
> +    mTelephonyAudioAgent->StopPlaying();
> +  } else {
> +    float volume = 1.0;
> +    bool muted = false;
> +    mTelephonyAudioAgent->StartPlaying(&volume, &muted);

1. nsresult rv = ..
2. do we care about volume and muted? write a comment for this.

@@ +629,5 @@
>  
>  NS_IMETHODIMP
>  Telephony::CallStateChanged(nsITelephonyCallInfo* aInfo)
>  {
> +  nsresult rv = HandleCallInfo(aInfo);

if (NS_WARN_IF(NS_FAILED(rv))) ...

@@ +630,5 @@
>  NS_IMETHODIMP
>  Telephony::CallStateChanged(nsITelephonyCallInfo* aInfo)
>  {
> +  nsresult rv = HandleCallInfo(aInfo);
> +  HandleTelephonyAudioAgentState();

if (NS_WARN_IF(NS_FAILED(rv))) ...

::: dom/telephony/Telephony.h
@@ +16,5 @@
>  
>  // Need to include TelephonyCall.h because we have inline methods that
>  // assume they see the definition of TelephonyCall.
>  #include "TelephonyCall.h"
> +#include "AudioChannelAgent.h"

remove this and use a forward declaration.

::: dom/webidl/Telephony.webidl
@@ +49,5 @@
>    [Throws]
>    void stopTone(optional unsigned long serviceId);
>  
>    [Throws]
> +  void enableTelephonyAudioAgent();

Add a comment here explaining that, calling this method, the app will be treated as owner of the telephony calls from the AudioChannel policy.

	Szu-Yu Chen [:aknow] (inactive)
	Comment 4 • 9 years ago

Comment on attachment 8559744 [details] [diff] [review]
Create new function and audio agent in telephony object

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

::: dom/telephony/Telephony.cpp
@@ +524,5 @@
>    aRv = mService->StopTone(serviceId);
>  }
>  
> +void
> +Telephony::EnableTelephonyAudioAgent(ErrorResult& aRv)

From the implementation, it looks like no exception will be thrown in this function?  Are you going to throw something or remove "Throws" from webidl description.

@@ +540,5 @@
> +  if (!mTelephonyAudioAgent) {
> +    return;
> +  }
> +
> +  Nullable<OwningTelephonyCallOrTelephonyCallGroup> isTelephonyAudioActive;

nit: naming. 
'isFoo' indicates it's a boolean value, but it's not here.  I'd prefer to use something like 'activeCall'.

::: dom/webidl/Telephony.webidl
@@ +49,5 @@
>    [Throws]
>    void stopTone(optional unsigned long serviceId);
>  
>    [Throws]
> +  void enableTelephonyAudioAgent();

nit: naming. 

1. Basically, we don't need the 'Telephony' prefix because everything here is about Telephony. 
2. I think that maybe "enableAudio" is enough.  "AudioAgent" just provides too much details for API users.

Do we need a method for "disable..."?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 5 • 9 years ago

Comment on attachment 8559744 [details] [diff] [review]
Create new function and audio agent in telephony object

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

I'd like to second Aknow's comments on the general naming and the question about "disable" API.

::: dom/telephony/Telephony.cpp
@@ +638,5 @@
>  NS_IMETHODIMP
>  Telephony::EnumerateCallState(nsITelephonyCallInfo* aInfo)
>  {
>    return HandleCallInfo(aInfo);
>  }

We need to call |HandleTelephonyAudioAgentState();| in |EnumerateCallStateComplete|, too.

::: dom/telephony/Telephony.h
@@ +94,5 @@
>  
>    void
>    StopTone(const Optional<uint32_t>& aServiceId, ErrorResult& aRv);
>  
> +  // In the new audio channel architecture, the system app need to know the

nit: s/need/needs/

@@ +95,5 @@
>    void
>    StopTone(const Optional<uint32_t>& aServiceId, ErrorResult& aRv);
>  
> +  // In the new audio channel architecture, the system app need to know the
> +  // state of every audio channels, including the telepony. Therefore, when

nit: s/channels/channel/

@@ +96,5 @@
>    StopTone(const Optional<uint32_t>& aServiceId, ErrorResult& aRv);
>  
> +  // In the new audio channel architecture, the system app need to know the
> +  // state of every audio channels, including the telepony. Therefore, when
> +  // the telephony start, the audio channel service would notify the system

nit: s/start/starts/

@@ +98,5 @@
> +  // In the new audio channel architecture, the system app need to know the
> +  // state of every audio channels, including the telepony. Therefore, when
> +  // the telephony start, the audio channel service would notify the system
> +  // app about that. And we need a agent to communicate with the audio channel
> +  // service. we would follow the call states to make a correct notification.

nit: s/we/We/

	Alastor Wu [:alwu] Assignee
	Comment 6 • 9 years ago

Hi, Baku 
> 
> no callbacks? It means that we cannot mute/unmute/change the volume for the telephony channel?
> 
Oops, is my fault, I forgot to add that.
I think that we should directly change the volume in the AudioChannelSerivce instead of use the callbacks function. 

Because the volume changing of the telephony is only happening in the AudioManager, it seems not proper to use the AudioManager in the telephony object.

the code maybe like this,

void AudioChannelService::SetAudioChannelVolume(...) 
{
  // do something
  if (aAudioChannel == AudioChannel::Telephony) {
    if (IsParentProcess()) {
      SetTelephonyVolume(aVolume);
    } else {
      ContentChild* cc = ContentChild::GetSingleton();
      if (cc) {
        cc->SetTelephonyVolume(aVolume);
      }
    }
  }
}

How do you think?

---

Hi, Aknow & Hsin-Yi,
> 
> Do we need a method for "disable..."?
> 
The enable() is to create an audio agent, and the |HandleTelephonyAudioAgentState()| would follow the call state to decide whether registers/unregisters this agent. When there is no call existing, the agent would not register to the AudioChannelService. Therefore, we don't need a disable(), because the agent would not do anything when there is no call.

How do you think?

---

Thanks you all for giving me suggestions :)

	Andrea Marchesini [:baku]
	Comment 7 • 9 years ago

> I think that we should directly change the volume in the AudioChannelSerivce
> instead of use the callbacks function. 

mmm... I don't like it. Why cannot we do this in the Telephony object?
The AudioChannelService doesn't touch the volume for any other audioChannel.

It seems to me that the telephony obj can do it, perfectly.

>       SetTelephonyVolume(aVolume);

Where is it implemented?


> the agent would not register to the AudioChannelService. Therefore, we don't
> need a disable(), because the agent would not do anything when there is no
> call.
> 
> How do you think?

Correct. When you call StopPlaying() the AudioChannelAgent disconnects itself.

	Alastor Wu [:alwu] Assignee
	Comment 8 • 9 years ago

Hi, Baku,
Now I change the audio volume in the telephony object.
When the agent receives the volume changing notification, it would use the AudioManager's functions to set the stream volume. 
Still thanks a lots :)

	Andrea Marchesini [:baku]
	Comment 9 • 9 years ago

Comment on attachment 8561161 [details] [diff] [review]
Create new function and audio agent in telephony object

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

looks good.

::: dom/telephony/Telephony.cpp
@@ +30,5 @@
>  // Service instantiation
>  #include "ipc/TelephonyIPCService.h"
>  #if defined(MOZ_WIDGET_GONK) && defined(MOZ_B2G_RIL)
>  #include "nsIGonkTelephonyService.h"
> +#include "nsIAudioManager.h"

alphabetic order

@@ +39,1 @@
>  

here too.

@@ +531,5 @@
> +{
> +  if (!mAudioAgent) {
> +    mAudioAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
> +    MOZ_ASSERT(mAudioAgent);
> +    nsresult rv = mAudioAgent->InitWithWeakCallback(GetParentObject(),

aRv = mAudioAgent-> ...

@@ +534,5 @@
> +    MOZ_ASSERT(mAudioAgent);
> +    nsresult rv = mAudioAgent->InitWithWeakCallback(GetParentObject(),
> +                                                   (int32_t)AudioChannel::Telephony,
> +                                                    this);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

if (NS_WARN_IF(aRv.Failed()) {
  return;
}

@@ +556,5 @@
> +    // These two varibles are meaningless, we just want to register this agent
> +    // to the audio channel service.
> +    float volume = 1.0;
> +    bool muted = false;
> +    nsresult rv = mAudioAgent->StartPlaying(&volume, &muted);

check if WindowVolumeChanged() is called. If not, do: WindowVolumeChanged(volume, muted); and remove the comment.

@@ +571,5 @@
> +  // The telephony audio is only allowed to run on the main process now.
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +
> +  nsCOMPtr<nsIAudioManager> audioManager = do_GetService(NS_AUDIOMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(audioManager, NS_OK);

I think we want to remove NS_ENSURE_TRUE. Just write:

if (NS_WARN_IF(!audioManager)) {
  return NS_OK;
}

@@ +574,5 @@
> +  nsCOMPtr<nsIAudioManager> audioManager = do_GetService(NS_AUDIOMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(audioManager, NS_OK);
> +
> +  int32_t volume = (aMuted)? 0 : (int32_t) aVolume;
> +  audioManager->SetAudioChannelVolume((int32_t)AudioChannel::Telephony, volume);

nsresult rv = ..
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +662,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  return HandleAudioAgentState();

rv = HandleAudioAgentState();
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

return NS_OK;

@@ +673,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  return HandleAudioAgentState();

here too.

::: dom/telephony/Telephony.h
@@ +16,5 @@
>  
>  // Need to include TelephonyCall.h because we have inline methods that
>  // assume they see the definition of TelephonyCall.
>  #include "TelephonyCall.h"
> +#include "nsIAudioChannelAgent.h"

move this at line 14. Alphabetic order.

@@ +97,5 @@
>  
>    void
>    StopTone(const Optional<uint32_t>& aServiceId, ErrorResult& aRv);
>  
> +  // In the new audio channel architecture, the system app need to know the

1. heheh... the 'new audio channel architecture', will it be new for ever? :)
Maybe just say: "In the audio channel architecture..."

2. the system app needs

@@ +98,5 @@
>    void
>    StopTone(const Optional<uint32_t>& aServiceId, ErrorResult& aRv);
>  
> +  // In the new audio channel architecture, the system app need to know the
> +  // state of every audio channel, including the telepony. Therefore, when

telephony

@@ +99,5 @@
>    StopTone(const Optional<uint32_t>& aServiceId, ErrorResult& aRv);
>  
> +  // In the new audio channel architecture, the system app need to know the
> +  // state of every audio channel, including the telepony. Therefore, when
> +  // the telephony starts, the audio channel service would notify the system

// a telephony call is activated, ...

@@ +228,5 @@
>    nsresult
>    HandleCallInfo(nsITelephonyCallInfo* aInfo);
> +
> +  // Check the call states to decide whether need to send the notificaiton.
> +  nsresult HandleAudioAgentState();

nsresult<br />HandleAudioAgentState();

::: dom/webidl/Telephony.webidl
@@ +51,5 @@
>  
> +  // Calling this method, the app will be treated as owner of the telephony
> +  // calls from the AudioChannel policy.
> +  [Throws]
> +  void enableAudio();

maybe we should rename it. What about: 'ownAudioChannel' ?

	Szu-Yu Chen [:aknow] (inactive)
	Comment 10 • 9 years ago

Comment on attachment 8561161 [details] [diff] [review]
Create new function and audio agent in telephony object

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

I still have the question for 'enableXXX' without 'disableXXX'?  Is it possible for the API user to decide to not own the audio channel when there are active calls?  It it is the case, we'll need that 'disableXXX'

::: dom/telephony/Telephony.cpp
@@ +533,5 @@
> +    mAudioAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
> +    MOZ_ASSERT(mAudioAgent);
> +    nsresult rv = mAudioAgent->InitWithWeakCallback(GetParentObject(),
> +                                                   (int32_t)AudioChannel::Telephony,
> +                                                    this);

I think you'll need to call HandleAudioAgentState() here.
It's possible that API user call the function when several calls are already established.

@@ +564,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP

Not here.  Move the function to the place above this line.
https://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp?from=Telephony.cpp&case=true#599

Also, add a comment before it to indicate that's the place for nsIAudioChannelAgentCallback.
"// nsIAudioChannelAgentCallback"

::: dom/telephony/Telephony.h
@@ +36,3 @@
>                              private nsITelephonyListener
>  {
> +  NS_DECL_NSIAUDIOCHANNELAGENTCALLBACK

Not here.  Place it with other "NS_DECL_NSI*"

@@ +231,5 @@
> +  // Check the call states to decide whether need to send the notificaiton.
> +  nsresult HandleAudioAgentState();
> +
> +  // The audio agent is needed to communication with the audio channel service.
> +  nsCOMPtr<nsIAudioChannelAgent> mAudioAgent;

Not here.  Declare it in the beginning part of the class.  That's the style for this class

	Andrea Marchesini [:baku]
	Comment 11 • 9 years ago

I think this patch can land when ready without waiting for the new AudioChannel API.

	Alastor Wu [:alwu] Assignee
	Comment 12 • 9 years ago

Hi, Aknow & Baku,
Here is my revised patch.

Avoid to confuse the developers, changing the function name from enable..() to own..(). That we can only has one function instead of having both enable/disable.

Very appreciate for your help!

	Alastor Wu [:alwu] Assignee
	Comment 13 • 9 years ago

Hi, Baku,
Could you give me some suggestions?
I'm not sure whether what I did is the correct...

And here are some questions,
Why these events didn't be defined in the HTMLMediaElement.webidl, but only in AudioContent.webidl? 
What is the difference between defining events in the nsGkAtomList.h and in the webidl?

Thanks a lots :)

	Szu-Yu Chen [:aknow] (inactive)
	Comment 14 • 9 years ago

Comment on attachment 8562504 [details] [diff] [review]
Create new function and audio agent in telephony object

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

r=me with comments addressed

::: dom/telephony/Telephony.cpp
@@ +535,5 @@
> +    aRv = mAudioAgent->InitWithWeakCallback(GetParentObject(),
> +                                           (int32_t)AudioChannel::Telephony,
> +                                            this);
> +    HandleAudioAgentState();
> +    if (NS_WARN_IF(aRv.Failed())) {

I think that you should have this check before calling HandleAudioAgentState().

Also you have to check the return value of HandleAudioAgentState().

What happened for the subsequent HandleAudioAgenState() (triggered by call state changed) if we got fail from InitWithWeakCallback()?

	Andrea Marchesini [:baku]
	Comment 15 • 9 years ago

Comment on attachment 8562504 [details] [diff] [review]
Create new function and audio agent in telephony object

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

::: dom/telephony/Telephony.cpp
@@ +533,5 @@
> +    mAudioAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
> +    MOZ_ASSERT(mAudioAgent);
> +    aRv = mAudioAgent->InitWithWeakCallback(GetParentObject(),
> +                                           (int32_t)AudioChannel::Telephony,
> +                                            this);

if (NS_WARN_IF(aRv.Failed()) ...

@@ +534,5 @@
> +    MOZ_ASSERT(mAudioAgent);
> +    aRv = mAudioAgent->InitWithWeakCallback(GetParentObject(),
> +                                           (int32_t)AudioChannel::Telephony,
> +                                            this);
> +    HandleAudioAgentState();

aRv = HandleAudioAgentState();
if (NS_WARN_IF(...

@@ +552,5 @@
> +  Nullable<OwningTelephonyCallOrTelephonyCallGroup> activeCall;
> +  GetActive(activeCall);
> +  nsresult rv;
> +  if (activeCall.IsNull()) {
> +    rv = mAudioAgent->StopPlaying();

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +560,5 @@
> +    rv = mAudioAgent->StartPlaying(&volume, &muted);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +    rv = WindowVolumeChanged(volume, muted);

if (NS_WARN_IF(NS_FAILED(rv))) ...

@@ +562,5 @@
> +      return rv;
> +    }
> +    rv = WindowVolumeChanged(volume, muted);
> +  }
> +  return rv;

return NS_OK;

@@ +644,5 @@
> +NS_IMETHODIMP
> +Telephony::WindowVolumeChanged(float aVolume, bool aMuted)
> +{
> +  // The telephony audio is only allowed to run on the main process now.
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

Do we have this check somewhere else too, right? Maybe in the constructor... ?

@@ +647,5 @@
> +  // The telephony audio is only allowed to run on the main process now.
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +
> +  nsCOMPtr<nsIAudioManager> audioManager = do_GetService(NS_AUDIOMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(audioManager, NS_OK);

if (NS_WARN_IF(!audioManager))) {
  return NS_OK;
}

@@ +665,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  return HandleAudioAgentState();

rv = HandleAudioAgentState();
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

return NS_OK;

@@ +676,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  return HandleAudioAgentState();

same here.

	Andrea Marchesini [:baku]
	Comment 16 • 9 years ago

Comment on attachment 8562510 [details] [diff] [review]
Add moz-interrupt-begin/end event

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

::: dom/telephony/Telephony.cpp
@@ +659,5 @@
> +    DispatchMediaEvent(NS_LITERAL_STRING("mozinterruptbegin"));
> +    audioManager->SetAudioChannelVolume((int32_t)AudioChannel::Telephony,
> +                                        (int32_t) aVolume);
> +  }
> +

here I would do:

audioManager->SetAudioChannelVolume((int32_t)AudioChannel::Telephony, volume);

if (mMuted != aMuted) {
  mMuted = aMuted;
  DispatchMediaEvent(mMuted ? NS_LITERAL_STRING("mozinterruptbegin") : NS_LISTERAL_STRING("mozinterruptend"));
}

and initialize mMuted in the ctor to false.

	Alastor Wu [:alwu] Assignee
	Comment 17 • 9 years ago

Hi, Aknow,
If the InitWithWeakCallback() fails, we would not have the audio agent. That means the system app can not use the audio channel to control the volume of the telephony audio channel. 

Hi, Baku,
I thought that we will initialize all audio channel to muted, then wait for the playable permission from the system app, doesn't it?

	Alastor Wu [:alwu] Assignee
	Comment 18 • 9 years ago

Hi, Baku
This patch is based on the assumption that we initialize the audio channel muted until receiving the playable permission from the system app.
Sorry to bother you a lots, very appreciate!!

	Alastor Wu [:alwu] Assignee
	Comment 19 • 9 years ago

Sorry for missing some error handlings on previous patch.
It is the revised version.

>
> Do we have this check somewhere else too, right? Maybe in the constructor... ?
>

This can't be add in the constructor, because the app in the child process also can get the telephony object. I add the check in other audio-channel-related functions. 

Thanks :)

	Andrea Marchesini [:baku]
	Comment 20 • 9 years ago

Comment on attachment 8563872 [details] [diff] [review]
Create new function and audio agent in telephony object

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

Can you please answer my questions? Then ask a second review. Thanks!

::: dom/telephony/Telephony.cpp
@@ +528,5 @@
>  
> +void
> +Telephony::OwnAudioChannel(ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

Actually, there is nothing preventing Telephony obj to be used in the child process.
We should support it here too.
I guess you can just remove this assertion and write a mochitest!

@@ +548,5 @@
> +
> +nsresult
> +Telephony::HandleAudioAgentState()
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

here as well.

@@ +658,5 @@
> +NS_IMETHODIMP
> +Telephony::WindowVolumeChanged(float aVolume, bool aMuted)
> +{
> +  // The telephony audio is only allowed to run on the main process now.
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

Maybe I'm wrong... can you tell me where we do this check elsewhere?

@@ +690,5 @@
>  
>  NS_IMETHODIMP
>  Telephony::EnumerateCallState(nsITelephonyCallInfo* aInfo)
>  {
> +  nsresult rv = HandleCallInfo(aInfo);

just do:

return CallStateChanged(aInfo);

	Andrea Marchesini [:baku]
	Comment 21 • 9 years ago

Comment on attachment 8563870 [details] [diff] [review]
Add moz-interrupt-begin/end event

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

::: dom/telephony/Telephony.cpp
@@ +679,5 @@
>    }
>  
> +  // These events will be triggered when the telephony is interrupted by other
> +  // audio channel. But we should not fire "mozinterruptend" when system app
> +  // intialize the telephony audio from muted to unmuted at the first time.

I don't understand. Tell me more. So, at the beginning the telephony obj is muted... right?
When the telephony app will unmute it and we should not dispatch mozinterrupt-end.

So, If this is what you want to do, I guess we should just not do it.
First of all the Telephony object should not know how  the system app works.

Just set it unmuted by default. The AudioChannel will mute it and we will dispatch a mozinterruptbegin.
When the system app will unmute it, we will have a mozinterruptend.

It's up to the application deal with this behavior.

What do you think about this?

::: dom/telephony/Telephony.h
@@ +171,5 @@
>  private:
>    explicit Telephony(nsPIDOMWindow* aOwner);
>    ~Telephony();
>  
> +  bool

same line.

@@ +175,5 @@
> +  bool
> +  mMuted;
> +
> +  bool
> +  mIsInitUnmuted;

here too.

	Alastor Wu [:alwu] Assignee
	Comment 22 • 9 years ago

Hi, Baku,
Sorry I don't understand very well...
You means that I should remove the assertion to keep the consistent of the code?

In my understanding, the telephony object can be used by anyone. But if we want to control the sound, the new functions we added should only be used in the parent process. Because audio manager can only be used in the parent process, and it is the only way to directly control the volume of the telephony stream. Is that right? 

I thought that the main drawback of the old audio channel design is to playback the new audio first, then stop the old audio. It results some sound leaking issue, doesn't it? Why don't we keep all audio channels muted by default, and let the system app to decide the playable audio channel? We could ensure the new audio playbacks after the old audio stops. 

How do you think :)?
Very appreciate!!

	Andrea Marchesini [:baku]
	Comment 23 • 9 years ago

> In my understanding, the telephony object can be used by anyone. But if we
> want to control the sound, the new functions we added should only be used in
> the parent process.

Correct. But we cannot assume that the app knows the kind of process it is into.
We must support telephony in the child process. For this I guess you must use IPDL.

So, here what I would do:

in OwnAudioChannel don't add any assertion. nsIAudioChannelAgent knows how to work in a child process.
Same for  HandleAudioAgentState and WindowVolumeChanged. But when you want to use the AudioChannelManager you should do this:

int32_t volume = (aMuted)? 0 : (int32_t) aVolume;

if (XRE_GetProcessType() == GeckoProcessType_Default) {
  nsCOMPtr<nsIAudioManager> audioManager = do_GetService(NS_AUDIOMANAGER_CONTRACTID);
  ...
  audioManager->SetAudioChannelVolume((int32_t)AudioChannel::Telephony, volume);
} else {
  ContentChild* cc = ContentChild::GetSingleton();
  cc->SendTelephonyAudioChannelVolume(volume);
}


Then in dom/ipc/PContent.idl add in the parent section:

  async TelephonyAudioChannelVolume(int32_t aVolume);

and implement in ContentParent.{h,cpp}:

  bool RecvTelephonyAudioChannelVolume(const int32_t& aVolume);

and in this method you should do:

1. check if the app has the telephony permission
2. call AudioManager->SetAudioChannelVolume(int32_t)AudioChannel::Telephony,  the volume);

What do you think about this approach?

> Why don't we keep all audio channels muted by
> default, and let the system app to decide the playable audio channel? We
> could ensure the new audio playbacks after the old audio stops. 

About this I think that we should have the same behavior in WebAudio, HTMLMediaElements and telephony objects.
So here what I think we should do:

1. by default all these objects are not muted.
2. The BrowserElement API will mute them immediately if we are in b2g.
3. The system app will implement the correct logic.

But we should not have a logic to prevent the dispatching of mozinterruptbegin/end in telephony. Because it also maybe sense that the app receives mozinterruptend without having a mozinterruptbegin.

	Alastor Wu [:alwu] Assignee
	Comment 24 • 9 years ago

Hi, Baku,
Your approach sounds well, and the patch is WIP :)

---

Could I muted the audio in the AudioChannelService instead of the BrowserElement API? Like this,

>    AudioChannelConfig()
>      : mMuted(false)
>    {
>#ifdef MOZ_B2G
>      mMuted = true;
>#endif
>    }

---

I think we can force that the "mozinterruptend" must be dispatched after the "mozinterruptbegin". If this rule is available, we can avoid to dispatch the mozinterruptend without having a mozinterruptbegin.

And I want to know could we dispatch same event multiple times? 
ex. three times mozinterruptbegin, but no mozinterruptend.

What do you think about this approach?
Thanks :)

	Andrea Marchesini [:baku]
	Comment 25 • 9 years ago

> >    AudioChannelConfig()
> >      : mMuted(false)
> >    {
> >#ifdef MOZ_B2G
> >      mMuted = true;
> >#endif
> >    }

Yes we can do it.

> And I want to know could we dispatch same event multiple times? 
> ex. three times mozinterruptbegin, but no mozinterruptend.

No please :)
Use a boolean to avoid this.
Waiting for your patch!

	Alastor Wu [:alwu] Assignee
	Comment 26 • 9 years ago

I found that directly changing the telephony stream volume to zero doesn't stop the telephony sound.
When I change the volume to ZERO, the "voice_call" stream still keep the minimum volume 0.008, which I can see by the command "dumpsys media.audio_policy". It seems that we can't achieve ZERO volume of telephony in B2G. Even we press the hw button down, the volume can't change to ZERO. (the minimum telephony volume we can see from UI is one)

To stop the sound means that the phone call becomes paused state instead of ZERO volume. 

Now we use the audio context as a fake telephony element in the audio_competing_helper.js. If I try to set this element muted, it can stop the telephony sound successfully. But my code can't.

So I am trying to understand what is the difference between them, and how to stop the telephony sound without changing its volume.

	Alastor Wu [:alwu] Assignee
	Comment 27 • 9 years ago

Update the version of setting stream volume. (Keep record)
I will update another patch which is to changes the call states to hold the phone.

	Alastor Wu [:alwu] Assignee
	Comment 28 • 9 years ago

Hi, Aknow & Baku,

Because of hardware limitation, we can't muted telephony by setting its volume to ZERO. The minimum stream volume is hardware-dependant, and it is 0.008 in the flame-kk.

Therefore, I change the call states to achieve muting telephony. This muted method is only using by the system app to decide the audio competing policy. 

Ex. first call is GSM, second call is WebRTC. The system app muted the first call before the second call start.

	Alastor Wu [:alwu] Assignee
	Comment 29 • 9 years ago

Hi, Baku,
Here is the patch about moz-interrupt-begin/end.
I change the default audio option to muted, and also modified on the WebAudio and HTMLMediaElements.

	Alastor Wu [:alwu] Assignee
	Comment 30 • 9 years ago

Hi, Aknow & Baku,
Sorry, I forgot to say thanks!
Very appreciate for your help :)

	Andrea Marchesini [:baku]
	Comment 31 • 9 years ago

Comment on attachment 8571285 [details] [diff] [review]
Create new function and audio agent in telephony object

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

it looks good, but please, answer the question and ask me for a second review. Thanks!

::: dom/telephony/Telephony.cpp
@@ +67,5 @@
>  
>  Telephony::Telephony(nsPIDOMWindow* aOwner)
> +  : DOMEventTargetHelper(aOwner),
> +    mIsAudioStartPlaying(false),
> +    mIsAudioStartPlaying(false),

no twice :)

@@ +74,1 @@
>  {

MOZ_ASSERT(aOwner);

@@ +537,5 @@
> +void
> +Telephony::OwnAudioChannel(ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsIPermissionManager> permissionManager = services::GetPermissionManager();
> +  if (!permissionManager) {

if (NS_WARN_IF(!permissionManager)) {
  aRv.Throw(NS_ERROR_FAILURE);
  return;
}

@@ +576,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +    mIsAudioStartPlaying = false;
> +  } else if (!activeCall.IsNull()) {

what about if we are already playing?
Should this be:

} else if (!activeCall.IsNull() && !mIsAudioStartPlaying) {

@@ +674,5 @@
> +  for (uint32_t i = 0; i < mCalls.Length(); i++) {
> +    nsRefPtr<TelephonyCall>& tempCall = mCalls[i];
> +    ErrorResult aRv;
> +    if (aMuted) {
> +      tempCall->Hold(aRv);

So, there the problem:

by default, on b2g, we mute any audioChannel by default. So, when you call WindowVolumeChanged() at line 587, aMuted will be 'true'.
And you put the call on hold. I don't think we  want this. Do we?

One possible solution is to add, at line 673:

if (aMuted == mMuted) {
  return NS_OK;
}

The main question is: what is the behavior we want here?

@@ +679,5 @@
> +    } else {
> +      tempCall->Resume(aRv);
> +    }
> +    if (NS_WARN_IF(aRv.Failed())) {
> +      return NS_OK;

why NS_OK?
This should be return aRv.ErrorCode();

Or otherwise, write a comment...

@@ +683,5 @@
> +      return NS_OK;
> +    }
> +  }
> +
> +  +  // These events will be triggered when the telephony is interrupted by other

additional +

@@ +687,5 @@
> +  +  // These events will be triggered when the telephony is interrupted by other
> +  // audio channel.
> +  if (mMuted != aMuted) {
> +    mMuted = aMuted;
> +    // We should not dispatch "mozinterruptend" when the system app intialize

initializes

::: dom/telephony/Telephony.h
@@ +45,5 @@
>    class Listener;
>  
>    friend class telephony::TelephonyDialCallback;
>  
> +  // The audio agent is needed to communication with the audio channel service.

to communicate

	Alastor Wu [:alwu] Assignee
	Comment 32 • 9 years ago

Hi, Baku,
Sorry for my mistake, but I am not sure I really understand your mean...
Do you mean to reply the questions in Comment20? or other else?

---

> what about if we are already playing?
> Should this be:
> } else if (!activeCall.IsNull() && !mIsAudioStartPlaying) {

You are right.

---

> The main question is: what is the behavior we want here?

I hope the call can be muted when it started, but it can't.(Hardware-limitation) I afraid that the sound is started before the system app closes another sound, so only thing I can do is to put the call on hold at the beginning.

Does it make sense? 

Thanks!

	Szu-Yu Chen [:aknow] (inactive)
	Comment 34 • 9 years ago

Comment on attachment 8571285 [details] [diff] [review]
Create new function and audio agent in telephony object

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

r- for the question of holding a call

::: dom/telephony/Telephony.cpp
@@ +69,5 @@
> +  : DOMEventTargetHelper(aOwner),
> +    mIsAudioStartPlaying(false),
> +    mIsAudioStartPlaying(false),
> +    mHaveDispatchedInterruptBeginEvent(false),
> +    mMuted(false)

These two lines should be put into another patch.
It took me some time to find the definition of them and realized that they are included in another patch.

@@ +81,5 @@
>  
>    mReadyPromise = promise;
> +  mAudioAgent = nullptr;
> +#ifdef MOZ_B2G
> +  mMuted = true;

#else
  mMuted = false

and remove it from initialization list
I'd prefer to put the related code together.

@@ +674,5 @@
> +  for (uint32_t i = 0; i < mCalls.Length(); i++) {
> +    nsRefPtr<TelephonyCall>& tempCall = mCalls[i];
> +    ErrorResult aRv;
> +    if (aMuted) {
> +      tempCall->Hold(aRv);

I don't think it's proper to hold the call just because you want to mute the volume. When you hold the call, it affect both parties, not only yourself but also the remote party. The remote one now cannot hear your voice and that will be a weird situation for him/her. So maybe we need UX's opinion for the behavior we want.

Also, both hold and resume return a promise. In some cases, the request might fail.

@@ +690,5 @@
> +    mMuted = aMuted;
> +    // We should not dispatch "mozinterruptend" when the system app intialize
> +    // the telephony audio from muted to unmuted at the first time. The event
> +    // "mozinterruptend" must be dispatched after the "mozinterruptbegin".
> +    if (mHaveDispatchedInterruptBeginEvent || mMuted) {

Could you write it more explicitly. For example:

if (!mHaveDispatchedInterruptBeginEvent && mMuted) {
  DispatchTrustedEvent(NS_LITERAL_STRING("mozinterruptbegin"));
  mHaveDispatchedInterruptBeginEvent = true;
} else if (mHaveDispatchedInterruptBeginEvent && !mMuted) {
  DispatchTrustedEvent(NS_LITERAL_STRING("mozinterruptend"));
  mHaveDispatchedInterruptBeginEvent = false;
}

@@ +693,5 @@
> +    // "mozinterruptend" must be dispatched after the "mozinterruptbegin".
> +    if (mHaveDispatchedInterruptBeginEvent || mMuted) {
> +      DispatchTrustedEvent(mMuted? NS_LITERAL_STRING("mozinterruptbegin") :
> +                                   NS_LITERAL_STRING("mozinterruptend"));
> +      mHaveDispatchedInterruptBeginEvent = mMuted ? true : false;

equivalent to
mHaveDispatchedInterruptBeginEvent = mMuted;

	Alastor Wu [:alwu] Assignee
	Comment 35 • 9 years ago

Hi, Aknow & Baku,

Let me re-explain why and when we muted/unmuted the telephony. These methods are used to decide the audio competing policy by the system app. Common users and apps can't use this functionality.

In b2g, we set all audio channel's default to muted. When the new audio want to be playback, the system app would stop the other audio first to prevent sound leaking, afterward the system unmute the new audio.

Therefore, there are two situations that the telephony is muted,
(1) At beginning
(2) Audio competing (GSM/CDMA v.s VOIP)

Assume that we put the phone call on hold as a muted method,

The first situation, the system app would resume telephony very quickly after it stop other audio. So the user may not really feel the call being on hold.

The second situation, after discuss with UX, if the first call is GSM/CDMA, the second call is VOIP, then we need to put the first call on hold. 

So I think to put the phone call on hold is workable and make sense, how do you think?
Thanks :)

Ps. Sorry for my previous patch (have some obvious errors), I will notice that next time.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 36 • 9 years ago

Comment on attachment 8571285 [details] [diff] [review]
Create new function and audio agent in telephony object

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

::: dom/telephony/Telephony.cpp
@@ +544,5 @@
> +
> +  uint32_t perm = nsIPermissionManager::UNKNOWN_ACTION;
> +  nsIPrincipal* principal = GetParentObject()->GetDoc()->NodePrincipal();
> +  permissionManager->TestExactPermissionFromPrincipal(principal,
> +    NS_LITERAL_CSTRING("audio-channel-telephony").get(), &perm);

Sorry to jump in: Why can't we simply use [CheckPermissions] in webidl?

	Alastor Wu [:alwu] Assignee
	Comment 37 • 9 years ago

Thanks Hsin-Yi, I would revise it :)

	Alastor Wu [:alwu] Assignee
	Comment 38 • 9 years ago

Hi, Aknow & Baku,
Do you have any suggestions about comment35?
If both of you agree that muting the telephony by putting a call on hold, I would update the revised patch.
Thanks :)

	Andrea Marchesini [:baku]
	Comment 39 • 9 years ago

About the second situation, yes, I agree.

About the first situation, I'm not a telephony API expert and I cannot take this decision :)
We need somebody who has more knowledge about this API and telephony in general. I guess that, putting a call on hold involves some GSM thing... and I don't know if it's good or bad.

Aknow, are you familiar with this API?

	Szu-Yu Chen [:aknow] (inactive)
	Comment 40 • 9 years ago

As long as the audio for telephony is the first priority, everything should be fine, because you'll never put a call on hold. So, based on the current situation, I agree with your proposal. However I just want to remind you the potential problems and limitations.

1. Holding a call may fail. You have to check the returned promise and the subsequent state change of that call. Only when the promise is resolved (this means the request is sent successfully) and the state of the call changed to 'held' (network inform you the state changes), the entire 'holding' process is successful.

2. However in CDMA, there is no 'held' state and we don't have a response from network that tell you whether the call is changed or not.

So, to handle both 1 and 2 well, at least, you need to check the return promise of |call.hold()| and |call.resume()|.

	Borja Salguero [:borjasalguero]
	Comment 41 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #39)
> About the second situation, yes, I agree.
> 
> About the first situation, I'm not a telephony API expert and I cannot take
> this decision :)
> We need somebody who has more knowledge about this API and telephony in
> general. I guess that, putting a call on hold involves some GSM thing... and
> I don't know if it's good or bad.
> 
> Aknow, are you familiar with this API?

Hi Andrea,
We should take into consideration that currently we have a different behaviour depending if the call is a regular GSM call, or a Conference call [1]. In the case of an incoming WebRTC call (Firefox Hello for example) when we are in a GSM call, we have 2 cases:
- If we are in a regular GSM call and we receive a WebRTC incoming call, answering the WebRTC one must hold the GSM one [2]
- If we are in a Conference call, we don't let the WebRTC to hold our current GSM call (conference call has more priority than a regular call), so we 'compete' again in order to win the 'Audio Channel competition', so WebRTC call does not take place (this mechanism is implemented in Firefox Hello).

So after digging into the code it's Callscreen App the one who choose the right scenario. I don't know if this behavior can be moved to Gecko or not, but this is the current behaviour. Probably there is a UX spec for this, but I did not find it (Multiconference specs probably?). I hope it helps!


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L838
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L605
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L826

	Andrea Marchesini [:baku]
	Comment 42 • 9 years ago

> So after digging into the code it's Callscreen App the one who choose the
> right scenario. I don't know if this behavior can be moved to Gecko or not,
> but this is the current behaviour. Probably there is a UX spec for this, but
> I did not find it (Multiconference specs probably?). I hope it helps!

borjasalguero, yes! Thanks for your comment.
I think we can definitely use the telephony API to implement this behavior.

	Alastor Wu [:alwu] Assignee
	Comment 43 • 9 years ago

Hi, Aknow,
After checking the promise.h, I still don't know how to handle the promises which are returned by resume() and hold().

In general, we need to use the function Then() or Catch() to do something. But these functions are designed to be used in JS, they need a JSContext. It seems that I can't use them in my code. In addition, the promise doesn't provide the function to get its state directly. 

Therefore, I think that maybe we could check the error result to know whether we do the request successfully. Is that ok? 

How do you think?

---

Another question is I found that sometime I can't put the call on hold, even the return result is correct.

STR like that..
1. X make a first call to Y, Y answer the call, then mute Y 
  - X would listen the on hold voice (OK!)
  - Y become to on hold
2. X hang up the call
3. X make a second call to Y, Y answer the call, then mute Y 
  - X doesn't listen the on hold voice (Fail!)
  - Y has no change
  
The first call can be successful, but the second one can't.
Do you have any idea about that?

Very appreciate!!!!

	Szu-Yu Chen [:aknow] (inactive)
	Comment 44 • 9 years ago

The patch try to do some refactoring to solve the problem in Comment 43.

It looks like handling promise in cpp file is not so easy.  So my idea is to extract a function Hold(nsITelephonyCalback) for internal use.

Audio could define their own callback for Hold(callback)

class CustomCallback : public nsITelephonyCallback {
public:
  ...
  NS_IMETHODIMP NotifySuccess() {...}
  NS_IMETHODIMP NotifyError(const nsAString& aError) {...}
};

nsCOMPtr<nsITelephonyCallback> callback = new CustomCallback();
nsresult rv = someCall->Hold(callback);


Hsinyi,
Do you have any feedback for the proposal?

	Alastor Wu [:alwu] Assignee
	Comment 45 • 9 years ago

I try to fix the problem (mentioned on Comment43) that I can't put the phone on hold everytime.
I don't know the actually reason yet (WIP), I will ask for a review after finishing it .

	Alastor Wu [:alwu] Assignee
	Comment 46 • 9 years ago

Hi, Aknow & Baku,
Here is my revised patch.
The main difference is changing the function Resume() and Hold() so that they can return the error result correctly.
The fail of putting the call on holding is also fixed in this patch.

Very appreciate :)

	Alastor Wu [:alwu] Assignee
	Comment 47 • 9 years ago

Hi, Baku,
The reply for your previous review is on the comment32.
If you still have some questions or you think my reply is not enough, you can ask me to answer anytime, before you review my patch.
Thanks a lots :)

	Szu-Yu Chen [:aknow] (inactive)
	Comment 48 • 9 years ago

Comment on attachment 8580633 [details] [diff] [review]
Create new function and audio agent in telephony object

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

Sorry that I forgot to mention the limitation for holding a call in previous review.  It's the thing we need to be careful about.  Beside this, all other parts look good.  Thank you

::: dom/telephony/Telephony.cpp
@@ +14,5 @@
>  #include "mozilla/dom/TelephonyBinding.h"
>  
>  #include "nsCharSeparatedTokenizer.h"
>  #include "nsContentUtils.h"
> +#include "nsIDocument.h"

Where is the usage part?

@@ +76,5 @@
>    nsRefPtr<Promise> promise = Promise::Create(global, rv);
>    MOZ_ASSERT(!rv.Failed());
>  
>    mReadyPromise = promise;
> +  mAudioAgent = nullptr;

You can put this line in the initialization list.

@@ +530,5 @@
>  
> +void
> +Telephony::OwnAudioChannel(ErrorResult& aRv)
> +{
> +  if (!mAudioAgent) {

if (mAudioAgent) {
  return;
}

To save one indent level.

@@ +655,5 @@
> +
> +NS_IMETHODIMP
> +Telephony::WindowVolumeChanged(float aVolume, bool aMuted)
> +{
> +  for (uint32_t i = 0; i < mCalls.Length(); i++) {

Here. We only need to hold/resume the active call.
You can use:

Nullable<OwningTelephonyCallOrTelephonyCallGroup> activeCall;
GetActive(activeCall);

Which may be a call or a callGroup.
Both of them provide hold/resume method.


There is also an limitation we need to be careful about.  Network mantains at most two connections.  Each connection could be either a single call or a conference group.  When there are two connections at a time, holding one connection will automatically resume another one.  So, we could only switch between connections and it's impossible to hold all of them.

That means, in some cases (two connections), calls cannot be muted.  You could use following condition to check whether there are two connections:

mCalls.Length() > 1 || (mCalls.Length() == 1 && mGroup.CallsArray().Length())

	Andrea Marchesini [:baku]
	Comment 49 • 9 years ago

Comment on attachment 8580633 [details] [diff] [review]
Create new function and audio agent in telephony object

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

lgtm, but fix the szchen's comments and ask szchen to review it again.

::: dom/telephony/Telephony.cpp
@@ +76,5 @@
>    nsRefPtr<Promise> promise = Promise::Create(global, rv);
>    MOZ_ASSERT(!rv.Failed());
>  
>    mReadyPromise = promise;
> +  mAudioAgent = nullptr;

Actually, you can remove this line completely. mAudioAgent is a nsCOMPtr.

::: dom/telephony/TelephonyCall.cpp
@@ +295,5 @@
>    nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  nsCOMPtr<nsITelephonyCallback> callback = new TelephonyCallback(promise);
> +  nsresult rv = Hold(callback);

aRv = Hold(callback);
if (NS_WARN_IF(aRv.Failed()) {
  return nullptr;
}

@@ +327,5 @@
>    }
>  
> +  nsresult rv = mTelephony->Service()->HoldCall(mServiceId, mCallIndex, aCallback);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return NS_ERROR_FAILURE;

return rv;

@@ +354,5 @@
>    nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  nsCOMPtr<nsITelephonyCallback> callback = new TelephonyCallback(promise);
> +  nsresult rv = Resume(callback);

aRv = Resume(callback);
if (NS_WARN_IF(aRv.Failed()) {
  return nullptr;
}

@@ +385,5 @@
>    }
>  
> +  nsresult rv = mTelephony->Service()->ResumeCall(mServiceId, mCallIndex, aCallback);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return NS_ERROR_FAILURE;

return rv;

	Andrea Marchesini [:baku]
	Comment 50 • 9 years ago

Comment on attachment 8580635 [details] [diff] [review]
Add moz-interrupt-begin/end event

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

lgtm r=me.

::: dom/audiochannel/AudioChannelService.h
@@ +148,3 @@
>        , mNumberOfAgents(0)
> +    {
> +#ifdef MOZ_B2G

Sorry, I've changed my mind about this MOZ_B2G... what about if we have a pref?
I guess it's better if we have a 'dom.autochannel.mutedByDefault and have it set to true on b2g, and false everywhere else.
I just submitted a patch for this. Can you review it?

::: dom/html/HTMLMediaElement.cpp
@@ +4126,5 @@
>      SetMutedInternal(mMuted | MUTED_BY_AUDIO_CHANNEL);
>      DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptbegin"));
> +    mHaveDispatchedInterruptBeginEvent = true;
> +  } else if (!aMuted && (mMuted & MUTED_BY_AUDIO_CHANNEL) &&
> +             mHaveDispatchedInterruptBeginEvent) {

hold on, this means that the mediaelement is not actually muted if we didn't send the mozinterruptbegin.
I guess that what you want is:

} else if (!aMuted && (mMuted & MUTED_BY_AUDIO_CHANNEL)) {
  SetMutedInternal(mMuted & ~MUTED_BY_AUDIO_CHANNEL);

  if (mHaveDispatchedInterruptBeginEvent) {
    mHaveDispatchedInterruptBeginEvent = false;
    DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend"));
  }
}

::: dom/telephony/Telephony.cpp
@@ +79,5 @@
>  
>    mReadyPromise = promise;
>    mAudioAgent = nullptr;
> +#ifdef MOZ_B2G
> +  mMuted = true;

I guess we should expose a AudioChannelService::IsMutedByDefault(). Follow up.

	Alastor Wu [:alwu] Assignee
	Comment 51 • 9 years ago

(In reply to Szu-Yu Chen [:aknow] from comment #48)
> ::: dom/telephony/Telephony.cpp
> @@ +14,5 @@
> > +#include "nsIDocument.h"
> 
> Where is the usage part?

Sorry, I forgot to delete it.

> @@ +76,5 @@
> > +  mAudioAgent = nullptr;
> 
> You can put this line in the initialization list.

Delete this line.

> @@ +530,5 @@
> if (mAudioAgent) {
>   return;
> }
> 
> To save one indent level.

OK.

> @@ +655,5 @@
> Here. We only need to hold/resume the active call.
> You can use:
> 
> Nullable<OwningTelephonyCallOrTelephonyCallGroup> activeCall;
> GetActive(activeCall);
> 
> Which may be a call or a callGroup.
> Both of them provide hold/resume method.
> 
> 
> There is also an limitation we need to be careful about.  Network mantains
> at most two connections.  Each connection could be either a single call or a
> conference group.  When there are two connections at a time, holding one
> connection will automatically resume another one.  So, we could only switch
> between connections and it's impossible to hold all of them.
> 
> That means, in some cases (two connections), calls cannot be muted.  You
> could use following condition to check whether there are two connections:
> 
> mCalls.Length() > 1 || (mCalls.Length() == 1 && mGroup.CallsArray().Length())

Thanks for explain this!

But, here I think we still need to use the "mCalls" or "mGroup" instead of "activeCall".
Since we still need to resume the holding call when the interruption is ended.
If I use the activeCall here, I can't access the holding call from the activeCall.

	Alastor Wu [:alwu] Assignee
	Comment 52 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #49 and comment #50)
> ::: dom/telephony/Telephony.cpp
> @@ +76,5 @@
> > +  mAudioAgent = nullptr;
> 
> Actually, you can remove this line completely. mAudioAgent is a nsCOMPtr.

> ::: dom/telephony/TelephonyCall.cpp
> @@ +295,5 @@
> > +  nsCOMPtr<nsITelephonyCallback> callback = new TelephonyCallback(promise);
> > +  nsresult rv = Hold(callback);
> 
> aRv = Hold(callback);
> if (NS_WARN_IF(aRv.Failed()) {
>   return nullptr;
> }

> @@ +327,5 @@
> > +  nsresult rv = mTelephony->Service()->HoldCall(mServiceId, mCallIndex, aCallback);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return NS_ERROR_FAILURE;
> 
> return rv;

> @@ +354,5 @@
> > +  nsCOMPtr<nsITelephonyCallback> callback = new TelephonyCallback(promise);
> > +  nsresult rv = Resume(callback);
> 
> aRv = Resume(callback);
> if (NS_WARN_IF(aRv.Failed()) {
>   return nullptr;
> }

> @@ +385,5 @@
> > +  nsresult rv = mTelephony->Service()->ResumeCall(mServiceId, mCallIndex, aCallback);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return NS_ERROR_FAILURE;
> 
> return rv;

OK for above all.

> Sorry, I've changed my mind about this MOZ_B2G... what about if we have a
> pref?
> I guess it's better if we have a 'dom.autochannel.mutedByDefault and have it
> set to true on b2g, and false everywhere else.
> I just submitted a patch for this. Can you review it?
> 

OK, It seems that using the pref is better!
Sure! I'm very glad to review it! 
Btw, it is also my first review :) 

> hold on, this means that the mediaelement is not actually muted if we didn't
> send the mozinterruptbegin.
> I guess that what you want is:
> 
> } else if (!aMuted && (mMuted & MUTED_BY_AUDIO_CHANNEL)) {
>   SetMutedInternal(mMuted & ~MUTED_BY_AUDIO_CHANNEL);
> 
>   if (mHaveDispatchedInterruptBeginEvent) {
>     mHaveDispatchedInterruptBeginEvent = false;
>     DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend"));
>   }
> }

OK.

> I guess we should expose a AudioChannelService::IsMutedByDefault(). Follow
> up.

OK.

	Alastor Wu [:alwu] Assignee
	Comment 54 • 9 years ago

Hi, Aknow,
Here is my revised patch.
Thanks a lot for your help :)

	Szu-Yu Chen [:aknow] (inactive)
	Comment 55 • 9 years ago

Comment on attachment 8583682 [details] [diff] [review]
Create new function and audio agent in telephony object. r=baku.

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

Very Good!!  Thanks for your work.
r? hsinyi for the review of webidl (Telephony.webidl)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 56 • 9 years ago

Comment on attachment 8583682 [details] [diff] [review]
Create new function and audio agent in telephony object. r=baku.

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

Telephony.webidl looks good to me! Thank you.

	Alastor Wu [:alwu] Assignee
	Comment 57 • 9 years ago

Rebase the patch to latest version.

	Alastor Wu [:alwu] Assignee
	Comment 59 • 9 years ago

I also need to update some patches to move the Gaia workaround and modify some Gecko logic.

	Alastor Wu [:alwu] Assignee
	Comment 60 • 9 years ago

Correct comment59, I need to "remove" the Gaia workaround.

	Alastor Wu [:alwu] Assignee
	Comment 61 • 9 years ago

Hi, Gabriele,
Because we have already land the new audio channel architecture, and then we would need to land this bug.

This bug is about to attach the AudioChannelAgent to the callscreen app, so that the system app can control the telephony via the browser frame of the callscreen app.

Also, we implement the "moz-interrupt-begin/end" in the telephony object. Now we would automatically put the call on holding, when the system app want to pause it. 

Because of that, I think we can remove the audio_competing_helper.js [1] and the codes about putting the call on holding state [2]. As I know, the purpose of these codes is to listen the "mozinterrupt" and then to put the call on hold, all of these operations are already included in this patch and audio competing logic of the system app.

How do you think :)? Could we remove these codes?
Very appreciate!

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/audio_competing_helper.js
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L826

	Gabriele Svelto [:gsvelto] (PTO)
	Comment 62 • 9 years ago

Yes, I always saw that code as a terrible hack around something which should be handled properly elsewhere. I've got no qualms about seeing it go so feel free to open a bug for removing it; I'll write the change myself ASAP or if you prefer to do it yourself I'll provide the review. Naturally we'll have to keep our eyes peeled for regressions but as I've already mentioned in similar issues being relatively far from a release date makes this kind of changes appropriate at this stage.

	Alastor Wu [:alwu] Assignee
	Comment 63 • 9 years ago

Hi, Gabriele,
It's perfect if you can help me to remove these codes :)
Let's go to remove them and then use the new architecture to control the telephony!

	Alastor Wu [:alwu] Assignee
	Comment 64 • 9 years ago

Hi, Baku,
Here is the new patch to correct some errors.
Because we would set audio muted by default in b2g, if we don't have this patch, the MediaElement and AudioContext would send "mozinterruptbegin" when we set it to muted in the beginning, and send "mozinterruptend" when the system start to open it.
Thanks!

	Andrea Marchesini [:baku]
	Comment 65 • 9 years ago

Comment on attachment 8635139 [details] [diff] [review]
Only send mozinterrupt when interruption happens

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

I need to know where mHaveDispatchedInterruptBeginEvent is from before continuing.

::: dom/html/HTMLMediaElement.cpp
@@ +2056,5 @@
>      mLoadedDataFired(false),
>      mAutoplaying(true),
>      mAutoplayEnabled(true),
>      mPaused(true),
> +    mMuted(AudioChannelService::IsAudioChannelMutedByDefault()),

This is wrong. You should do something like

mMuted(AudioChannelService::IsAudioChannelMutedByDefault()
         ? MUTED_BY_AUDIO_CHANNEL : 0);

@@ +4471,5 @@
>    }
>  
> +  if (mMuted != aMuted) {
> +    // We have to mute this channel.
> +    if (aMuted && !(mMuted & MUTED_BY_AUDIO_CHANNEL)) {

rebase this patch. I changed something here yesterday.

@@ +4475,5 @@
> +    if (aMuted && !(mMuted & MUTED_BY_AUDIO_CHANNEL)) {
> +      SetMutedInternal(mMuted | MUTED_BY_AUDIO_CHANNEL);
> +      if (UseAudioChannelAPI() && !mHaveDispatchedInterruptBeginEvent) {
> +        DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptbegin"));
> +        mHaveDispatchedInterruptBeginEvent = true;

where is this from? It's not in m-c. From which bug has it been introduced?

::: dom/media/webaudio/AudioDestinationNode.cpp
@@ +321,5 @@
>                ChannelInterpretation::Speakers)
>    , mFramesToProduce(aLength)
>    , mAudioChannel(AudioChannel::Normal)
>    , mIsOffline(aIsOffline)
> +  , mMuted(AudioChannelService::IsAudioChannelMutedByDefault())

This looks ok but renamed to mAudioChannelAgentMuted

	Alastor Wu [:alwu] Assignee
	Comment 66 • 9 years ago

Hi, Baku,
The mHaveDispatchedInterruptBeginEvent is came from the second patch of this bug.
Here is the revised patch, could you help me review it?
Thanks!

	Alastor Wu [:alwu] Assignee
	Comment 67 • 9 years ago

try-server result, all green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f651891242e

	Alastor Wu [:alwu] Assignee
	Comment 68 • 9 years ago

Hi, Baku,
I would remove another null ringtone agent in the bug1184482.

	Andrea Marchesini [:baku]
	Comment 69 • 9 years ago

Comment on attachment 8635866 [details] [diff] [review]
Only send mozinterrupt when interruption happens

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

::: dom/html/HTMLMediaElement.cpp
@@ +4476,5 @@
>      mAudioChannelVolume = aVolume;
>      SetVolumeInternal();
>    }
>  
> +  if (mMuted != aMuted) {

I think what you want here is:

if (aMuted != ComputedMuted()) {

	Andrea Marchesini [:baku]
	Comment 70 • 9 years ago

Comment on attachment 8636933 [details] [diff] [review]
Remove null window telephony agent

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

We are going to remove this ringer AudioChannelAgent too, right?

	Alastor Wu [:alwu] Assignee
	Comment 71 • 9 years ago

Rebase.

	Alastor Wu [:alwu] Assignee
	Comment 72 • 9 years ago

Try server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62b5db0e5a3

	Alastor Wu [:alwu] Assignee
	Comment 73 • 9 years ago

Got some test case fails, need to check the fail reason.

	Alastor Wu [:alwu] Assignee
	Comment 74 • 9 years ago

The try result in comment67 is good, but the result in comment 72 has some fails. The difference between two results is to add patch, "remove null window telephony agent".

I think I should file another bug to handle this patch, and land other patches first.

	Alastor Wu [:alwu] Assignee
	Comment 76 • 9 years ago

The try result is in the comment67.

	Carsten Book [:Tomcat]
	Comment 77 • 9 years ago

needs rebasing it seems:

applying muteLogic.patch
patching file dom/html/HTMLMediaElement.cpp
Hunk #2 FAILED at 4438
1 out of 2 hunks FAILED -- saving rejects to file dom/html/HTMLMediaElement.cpp.rej
patching file dom/media/webaudio/AudioDestinationNode.cpp
Hunk #1 FAILED at 316
Hunk #2 FAILED at 486
2 out of 2 hunks FAILED -- saving rejects to file dom/media/webaudio/AudioDestinationNode.cpp.rej
patching file dom/media/webaudio/AudioDestinationNode.h
Hunk #1 FAILED at 99
1 out of 1 hunks FAILED -- saving rejects to file dom/media/webaudio/AudioDestinationNode.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh muteLogic.patch

	Alastor Wu [:alwu] Assignee
	Comment 78 • 9 years ago

Sorry, Ryan,
Here is the rebased patch.

	Pulsebot
	Comment 79 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/6df9cef6b3ed
https://hg.mozilla.org/integration/b2g-inbound/rev/c17987dfa969
https://hg.mozilla.org/integration/b2g-inbound/rev/3620404c8725

	Carsten Book [:Tomcat]
	Comment 80 • 9 years ago

(In reply to Alastor Wu [:alwu] from comment #78)
> Created attachment 8638442 [details] [diff] [review]
> Only send mozinterrupt when interruption happens. r=baku.
> 
> Sorry, Ryan,
> Here is the rebased patch.

cool (i'm not Ryan but thats fine :) checkin done :)

	Carsten Book [:Tomcat]
	Comment 81 • 9 years ago

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2375532&repo=b2g-inbound

	Carsten Book [:Tomcat]
	Comment 82 • 9 years ago

seems also https://treeherder.mozilla.org/logviewer.html#?job_id=2376003&repo=b2g-inbound was caused by this change

	Pulsebot
	Comment 83 • 9 years ago

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/88100d9298eb
https://hg.mozilla.org/integration/b2g-inbound/rev/a646ced31606
https://hg.mozilla.org/integration/b2g-inbound/rev/fff93931f290

	Alastor Wu [:alwu] Assignee
	Comment 84 • 9 years ago

New try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aafc4e5504fc

	Alastor Wu [:alwu] Assignee
	Comment 85 • 9 years ago

Trying to fix the fails.
The reftest (webgl-capturestream-test.html) can be passed on my local environment. 
The Gaia unit test can't be started, trying to fix it.

	Alastor Wu [:alwu] Assignee
	Comment 86 • 9 years ago

I guess the test fails might be resulted by patch 2&3 (the modification related with the "moz-interrupt" in MediaElement/AudioDestinationNode).
I would remove them and file another bug to handle the "moz-interrupt" issue, because the modification of telephony part should be landed first ASAP.

	Alastor Wu [:alwu] Assignee
	Comment 88 • 9 years ago

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2d805fe78a3

	Pulsebot
	Comment 89 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/847b57aaeff7
https://hg.mozilla.org/integration/b2g-inbound/rev/9c6fde246f6e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 90 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/847b57aaeff7
https://hg.mozilla.org/mozilla-central/rev/9c6fde246f6e

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 91 • 9 years ago

This is causing a smoke test blocker and may need to be backed out. Please see bug 1193840.

	Pulsebot
	Comment 92 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/03d70e0295d4

	Alastor Wu [:alwu] Assignee
	Comment 93 • 9 years ago

The bug1185442 was fixed, we should land this again.
See bug1193840 comment7.

	Ryan VanderMeulen [:RyanVM]
	Comment 94 • 9 years ago

In the future, please reopen bugs when they're backed out.

	Pulsebot
	Comment 95 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5823a3df988
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e41bbb259cc

	Ryan VanderMeulen [:RyanVM]
	Comment 96 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/a5823a3df988
https://hg.mozilla.org/mozilla-central/rev/8e41bbb259cc

	[:fabrice] Fabrice Desré
	Comment 97 • 9 years ago

Once this is backed out, please run a full QA smoketest on a custom build before landing. thanks!

	Alastor Wu [:alwu] Assignee
	Comment 98 • 9 years ago

Very sorry for causing these regressions, please backout this, and then I will fix it.
It seems that the problem is not the same as my original thought.

	Ryan VanderMeulen [:RyanVM]
	Comment 99 • 9 years ago

For future reference, the sheriffing team isn't the group responsible for smoketest backouts. If it's not breaking CI, it's outside our team's scope. I've done it this time, but please go through proper channels in the future.

https://hg.mozilla.org/mozilla-central/rev/f3df9cd1701f

	Alastor Wu [:alwu] Assignee
	Comment 100 • 9 years ago

Bug 1129882 - create agent in telephony object.

	Alastor Wu [:alwu] Assignee
	Comment 101 • 9 years ago

Bug 1129882 - add mozInterrupt in telephony object. r=baku

	Alastor Wu [:alwu] Assignee
	Comment 103 • 9 years ago

Bug 1129882 - create agent in telephony object.

	Alastor Wu [:alwu] Assignee
	Comment 104 • 9 years ago

The patch 1 need to be reviewed again.

	Alastor Wu [:alwu] Assignee
	Comment 105 • 9 years ago

Since the time of the first review was longer time ago, and the RIL codebase changed a lots, I would ask for review again.

This patch is based on the comment56's patch.
We add the audio channel agent into the telephony object, so that the telephony can be managed by audio policy of the system app.

I also want to fix the regression error (bug1197852) in this patch. The root cause of the bug1197852 is that the resuming request was rejected by the telephony service.

In B2G, the system app manages audio playback policy. If there is a new sound want to be playback, it must wait for the permission from the system app. It means that the sound would be muted first, and then be unmuted. For telephony, the behaviors are hold() first, then resume(). However, the telephony service can't handle all these requests within a short period. The telephony service would reject our resume request, because the modem have not changed the call state yet. It causes that the telephony can't be resumed. 

After discussing with Ben offline, in present design, the telephony service have no solution for this situation. Therefore, we would let the telephony playable in the beginning.

---

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03664a2b37d4

	Alastor Wu [:alwu] Assignee
	Comment 106 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

Hi, Baku & Hsin-yi,
According to the comment105, I would like to ask for review again.
Could you help me review this patch?
Very appreciate :)

	Gabriele Svelto [:gsvelto] (PTO)
	Comment 107 • 9 years ago

We've hit bug 1198824 which I think was caused by the backout here (since we didn't backout bug 1185442 too), Alastor do you think that will be fixed once you land again? It's pretty urgent since it's a smoketest problem so if we can't get it fixed soon we might have to back out bug 1185442 and re-land when we're done here.

	Alastor Wu [:alwu] Assignee
	Comment 108 • 9 years ago

Hi, Gabriele,
Yes, this patch is the solution, I have post more info in the bug1198824.
Thanks!

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 109 • 9 years ago

https://reviewboard.mozilla.org/r/17277/#review15507

Thanks for the patch Alastor. Please see my comments!

::: dom/telephony/Telephony.h:108
(Diff revision 1)
> +  // system app about that. And we need a agent to communicate with the audio

s/a agent/an agent/

::: dom/telephony/Telephony.cpp:24
(Diff revision 1)
> +#include "AudioChannelAgent.h"

Do we really need this? If we do, please move it to line 9, before "mozilla/Preferences.h".

::: dom/telephony/Telephony.cpp:556
(Diff revision 1)
> +  // Only stop agent when the call is disconnected.

I'd like to make the comment more accurate. As there may be multiple calls, "the call" doesn't sound that right to me. Please say |Only stop the agent when there's no call.|

::: dom/telephony/Telephony.cpp:671
(Diff revision 1)
> +  // Check the limitation of the network connection

I'd like to see the comment is more meaningful and providing *why* we implement in this way. 

Please rewrite it as:
It's impossible to put all the calls on-hold in the multi-call case.

::: dom/telephony/Telephony.cpp:684
(Diff revision 1)
> +  // Check the single call or conference call

nit: The comment isn't that helpful as the code itself says everything. Perhaps just remove it.

::: dom/telephony/Telephony.cpp:730
(Diff revision 1)
> +  return CallStateChanged(currentCallNum, &aInfo);

EnumerateCallState() might be  called multiple times by gonk service; that means there will be multiple calls and we don't really want to |HandleAudioAgentState| per call. Instead, we should update audio agent state all at once when |EnumerateCallStateComplete|. So, don't change here, but call |HandleAudioAgentState()| in [1] instead.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp?offset=0#635

::: dom/telephony/TelephonyCall.h:15
(Diff revision 1)
> +#include "nsITelephonyService.h"

Add a new empty line between line 14 and 15.

::: dom/telephony/TelephonyCall.cpp:330
(Diff revision 1)
> +    return nullptr;

This changes the existing Hold WebAPI behaviour.
We should |return promise.forget();| to keep the original behaviour.

::: dom/telephony/TelephonyCall.cpp:347
(Diff revision 1)
> +    return nullptr;

This changes the existing Resume WebAPI behaviour.
We should |return promise.forget();| to keep the original behaviour.

::: dom/telephony/TelephonyCallGroup.cpp:353
(Diff revision 1)
> -    return promise.forget();
> +    return nullptr;

ditto.

::: dom/telephony/TelephonyCallGroup.cpp:372
(Diff revision 1)
> +    return nullptr;

ditto.

	Alastor Wu [:alwu] Assignee
	Comment 110 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

Bug 1129882 - create agent in telephony object.

	Alastor Wu [:alwu] Assignee
	Comment 111 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

Hi, Hsin-yi,
Thanks for your comment!
Here is my revised patch, could you help me review again?
Thanks :)

-- 

Hi, Baku,
According to the comment105, I can't mute the telephony by default.
Could you help me review this patch?
Thanks :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 112 • 9 years ago

https://reviewboard.mozilla.org/r/17277/#review15843

Hi Alastor, sorry that my previous comment wasn't 100% accurate. Please see the new ones.

::: dom/telephony/Telephony.h:17
(Diff revisions 1 - 2)
> +#include "AudioChannelService.h"

Please move this new added line before |#include "mozilla/dom/BindingDeclarations.h"|.

::: dom/telephony/TelephonyCall.cpp:330
(Diff revisions 1 - 2)
> -    return nullptr;
> +    return promise.forget();

Sorry my bad. My previous review comment wasn't 100% accurate.

To keep the existing API behaviour, we need to react according to different aRv results.

If the error code is NS_ERROR_DOM_INVALID_STATE_ERR then we |return promise.forget();| otherwise we return nullptr.

::: dom/telephony/TelephonyCall.cpp:347
(Diff revisions 1 - 2)
> -    return nullptr;
> +    return promise.forget();

ditto - If the error code is NS_ERROR_DOM_INVALID_STATE_ERR then we |return promise.forget();| otherwise we return nullptr.

::: dom/telephony/TelephonyCall.cpp:377
(Diff revisions 1 - 2)
>      return rv;

To make sure the value returned here can be distincted from NS_ERROR_DON_INVALID_STATE_ERR in line 347, let's return another specified code NS_ERROR_FAILURE.

::: dom/telephony/TelephonyCall.cpp:413
(Diff revisions 1 - 2)
>      return rv;

ditto - let's return another specified code NS_ERROR_FAILURE.

::: dom/telephony/TelephonyCallGroup.cpp:353
(Diff revisions 1 - 2)
> -    return nullptr;
> +    return promise.forget();

ditto - If the error code is NS_ERROR_DOM_INVALID_STATE_ERR then we |return promise.forget();| otherwise we return nullptr.

::: dom/telephony/TelephonyCallGroup.cpp:372
(Diff revisions 1 - 2)
> -    return nullptr;
> +    return promise.forget();

ditto - If the error code is NS_ERROR_DOM_INVALID_STATE_ERR then we |return promise.forget();| otherwise we return nullptr.

::: dom/telephony/TelephonyCallGroup.cpp:390
(Diff revisions 1 - 2)
>      return rv;

ditto - let's return another specified code NS_ERROR_FAILURE.

::: dom/telephony/TelephonyCallGroup.cpp:408
(Diff revisions 1 - 2)
>      return rv;

ditto - let's return another specified code NS_ERROR_FAILURE.

	Alastor Wu [:alwu] Assignee
	Comment 114 • 9 years ago

Thank Hsin-yi! 
I'll update the modified patch later.

	Alastor Wu [:alwu] Assignee
	Comment 115 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

Bug 1129882 - create agent in telephony object.

	Andrea Marchesini [:baku]
	Comment 116 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

https://reviewboard.mozilla.org/r/17279/#review15959

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 117 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

https://reviewboard.mozilla.org/r/17279/#review16137

Thanks for the work, Alastor!

::: dom/telephony/TelephonyCall.cpp:330
(Diff revisions 2 - 3)
> -    return promise.forget();
> +                !aRv.ErrorCodeIs(NS_ERROR_DOM_INVALID_STATE_ERR))) {

nit: indention

::: dom/telephony/TelephonyCall.cpp:348
(Diff revisions 2 - 3)
> -    return promise.forget();
> +                !aRv.ErrorCodeIs(NS_ERROR_DOM_INVALID_STATE_ERR))) {

ditto

::: dom/telephony/TelephonyCallGroup.cpp:353
(Diff revisions 2 - 3)
> -    return promise.forget();
> +                !aRv.ErrorCodeIs(NS_ERROR_DOM_INVALID_STATE_ERR))) {

ditto.

::: dom/telephony/TelephonyCallGroup.cpp:373
(Diff revisions 2 - 3)
> -    return promise.forget();
> +                !aRv.ErrorCodeIs(NS_ERROR_DOM_INVALID_STATE_ERR))) {

ditto.

	Alastor Wu [:alwu] Assignee
	Comment 118 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

Bug 1129882 - create agent in telephony object. r=baku, hsinyi

	Alastor Wu [:alwu] Assignee
	Comment 119 • 9 years ago

Comment on attachment 8652736 [details]
Patch 1 : create agent in telephony object. r=baku, hsinyi

Hi, KTucker,
Could you run a full QA smoketest on a my custom build before landing?
What files do I need to provide?
Thanks!

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 120 • 9 years ago

No-jun, can you please make us a custom build for this so we can run a smoke test? See the above comment.

	No-Jun Park [:njpark]
	Comment 121 • 9 years ago

Build should be available here:
https://tools.taskcluster.net/task-inspector/#OOgUD26UQuejoR8WpKpUOA/0

Just to note, I applied the complete diff from the mozreview request.  I think the other patch file is obsolete, since it failed to apply to gecko before or after applying the diff from the mozreview request.

patched build has the following;
https://github.com/npark-mozilla/gecko-dev/commit/111499e560ac9e4405cca68a08032d10564cba1b

	Alastor Wu [:alwu] Assignee
	Comment 122 • 9 years ago

Hi, No-jun,
Sorry, I forgot to rebase the patch2.
Here is the rebased patch, could you help me also apply this patch?
Very appreciate :)

	No-Jun Park [:njpark]
	Comment 125 • 9 years ago

Aha I see, I am rerunning the same job after pushing the 2nd patch to my local branch. The result should be available as Run 1.

2nd Commit location:
https://github.com/npark-mozilla/gecko-dev/commit/feee8b89453a42346f20753bd1b8a606c286f561

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 126 • 9 years ago

Alastor, we noticed that a song kept playing even though it was at the end of the song track and we saw some strange issues with Bluetooth. We need to investigate these issue further to see if they are occurring on the latest central build. We will give you a report on this tomorrow. Also, just so you know dial tones are not functioning properly due to bug 1201133.

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 127 • 9 years ago

This seems safe to land. We did not notice any further regressions on the custom build. The same smoke test blockers that were present on dogfood latest were also on the custom build. We did notice that bug 1201952 was not occurring on the custom build so it was actually better.

	Alastor Wu [:alwu] Assignee
	Comment 129 • 9 years ago

Thank KTucker and No-Jun!
According to the comment127, this patch is ready to land.

	Alastor Wu [:alwu] Assignee
	Comment 130 • 9 years ago

Hi, Ryan,
Could you help me check-in this patch?
Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 131 • 9 years ago

I'm no longer an active sheriff, sorry.

	Alastor Wu [:alwu] Assignee
	Comment 132 • 9 years ago

OK, thank ryan.

---

Hi, Wes,
Could you help me check-in this patch?
Thanks!

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 133 • 9 years ago

Just to be clear, which patch(es) need checked in?

	Alastor Wu [:alwu] Assignee
	Comment 134 • 9 years ago

Both of them need to be checked in, thanks!

	Carsten Book [:Tomcat]
	Comment 135 • 9 years ago

(In reply to Alastor Wu [:alwu] from comment #134)
> Both of them need to be checked in, thanks!

thats now done! did the checkin

	Pulsebot
	Comment 136 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/5624fdf4b0fe
https://hg.mozilla.org/integration/b2g-inbound/rev/5a91d8cf566d

	Carsten Book [:Tomcat]
	Comment 137 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/5624fdf4b0fe
https://hg.mozilla.org/mozilla-central/rev/5a91d8cf566d

	Chris Mills [:cmills]
	Comment 138 • 9 years ago

I've documented the new ownAudioChannel() method on these pages:

* https://developer.mozilla.org/en-US/docs/Web/API/Telephony#Methods (new Method listed on parent interface page)
* https://developer.mozilla.org/en-US/docs/Web/API/Telephony/ownAudioChannel (individual detail page for new method)

I've also added a mention about it to the FxOS 2.5 release notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Releases/2.5#Web_API_changes

Let me know if this is ok. A tech review would be great. Thanks!

	Alastor Wu [:alwu] Assignee
	Comment 139 • 9 years ago

Hi, Chris,
The document is good, thank for your help!

---

BTW, the Audio Channel API was changed a lots in bug1113086.
If we want to update its MDN, what should we do?
Will MDN team help to edit it? or we need to prepare a draft for it?
Thanks!

	Chris Mills [:cmills]
	Comment 140 • 9 years ago

(In reply to Alastor Wu [:alwu] from comment #139)
> Hi, Chris,
> The document is good, thank for your help!
> 
> ---
> 
> BTW, the Audio Channel API was changed a lots in bug1113086.
> If we want to update its MDN, what should we do?
> Will MDN team help to edit it? or we need to prepare a draft for it?
> Thanks!

Ideally, the best thing you could would be to produce a document like the following:

https://github.com/paulrouget/mozBrowserAPI/blob/master/BrowserAPI.md

That :paul created for the browser API - including webidl, and some quick demos and explanation of what the non obvious methods/properties/events do. This would make it much easier for me to put together the documentation on MDN.

I could also then watch the page and make updates as soon as I see that you've made any changes to the API.

I mean, there is this page:

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

Is this up-to-date? I can watch moWiki pages, so this might be suitable. This contains webidl at least.

I've created a card in our Trello docs priority board for AudioChannel, so at least it is on our radar. I'm not promising it'll get done really quickly, as we have such a huge backlog to do. We'll set priorities for Q4 docs soon.

	Alastor Wu [:alwu] Assignee
	Comment 141 • 9 years ago

Hi, Chris,

Because we just finished the huge refactor of the audio channel architecture, most info in the wiki are also out-of-date.
We'll update it after fixing all the regression errors resulted by refactoring. You can just ignore the audio channel until we update the wiki.
Thanks!

	Chris Mills [:cmills]
	Comment 142 • 9 years ago

(In reply to Alastor Wu [:alwu] from comment #141)
> Hi, Chris,
> 
> Because we just finished the huge refactor of the audio channel
> architecture, most info in the wiki are also out-of-date.
> We'll update it after fixing all the regression errors resulted by
> refactoring. You can just ignore the audio channel until we update the wiki.
> Thanks!

Ok, I will keep watching it so I can see when we are ready to start updating docs. Thanks to you as well!