Closed Bug 876631 Opened 11 years ago Closed 9 years ago

AudioChannelService refactoring

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files, 13 obsolete files)

61.01 KB, patch
Details | Diff | Splinter Review
17.10 KB, patch
mchen
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #754741 - Flags: review?(mchen)
Blocks: 853101
Hi Andrea,

It seems that AudioChannelAgent in FMRadio.cpp is not updated for new version.
b2g18?
(In reply to Andrea Marchesini (:baku) from comment #3)
> b2g18?

Yes, I applied your patch into b2g18. Or I need to apply it into v1.0.1?

More info: FMRadio.cpp used AudioChannelAgent and call setVisibilityState() but this API is replaced by setVisible(). Also the arguments for initWithWeakCallback() is different too, there is no window can be used to put in new version of initWithWeakCallback().
I updated the patch for 862899. This is needed too.
Comment on attachment 754741 [details] [diff] [review]
patch

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

Hi Andrea,

I didn't complete the review but there are some defect found.
So post the first round comments to you. Please help to correct them. Thanks.

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +12,5 @@
> +
> +void
> +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mChildDestroyed = true;

I think we need to take care the case of child process crashed then un-register this agent from AudioChannelService.

@@ +41,5 @@
> +void
> +AudioChannelAgentParent::NotifyAudioChannelStateChanged()
> +{
> +  AudioChannelService* service = AudioChannelService::GetAudioChannelService();
> +  bool muted = service->CanPlay(this);

1. CanPlay() returns true if this agent is allowed to play.
2. But muted variable means if true then this agent should not be allowed to play.
3. They are opposite meaning here. So after calling SendNotify(!muted) agentClient will become "not to play" even agentParent is allowed.

::: dom/audiochannel/AudioChannelService.cpp
@@ +153,5 @@
> +      // If nothing is visible, the list has to been frozen.
> +      // Or if there is still any one with other ChildID in foreground then
> +      // it should be removed from list and left other ChildIDs in the foreground
> +      // to keep playing. Finally only last one childID which go to background
> +      // will be in list.

In this comment we used childID to identify an app but in the code it seems we used groupID to identify a window or a tab. So we may need to modify the comment.

@@ +161,5 @@
> +        if (mAgents[i]->mAgent->Type() != AUDIO_CHANNEL_CONTENT) {
> +           continue;
> +        }
> +
> +        ++countContentAgents;

I think this variable is used to record any "visible" apps in the foreground and to set frozen to true if it is 0.
Based on this assumption, do we need to check whether this agent is hidden or not?

@@ +172,5 @@
> +
> +      if (!countContentAgents) {
> +        mActiveContentGroupIdsFrozen = true;
> +      } else if (!countVisibleContentAgentsInThisGroup) {
> +        mActiveContentGroupIds.RemoveElement(aAgent->GroupId());

maybe in comment we need to add something like this
  "The GroupID will be removed from list only when last agent moved from visible to hidden in case of multiple agents in the same GroupID"

@@ +213,5 @@
>  
> +  // This channel is supposed to play.
> +  if (mCurrentHighestChannel <= aAgent->Type()) {
> +    return true;
> +  }

In the case of current highest channel is content and aAgent is content too, then here will always return true.
But actually we need additional check in the next code section.

@@ +252,5 @@
>  
> +  for (int i = 0, len = mAgents.Length(); i < len; ++i) {
> +    bool canPlay = CanPlayInternal(mAgents[i]->mAgent, mAgents[i]);
> +    if (!canPlay) {
> +      continue;

To image the use case:
  1. Music app is on playing then alarm is fired.
  2. Alarm is end then calling UnregisterAudioChannelAgent() -> SendAudioChannelChangedNotification() to here.
  3. Agent with content channel can't be allowed to play because the highest channel is still alarm.
  4. Finally highest channel is remained to initial value - AUDIO_CHANNEL_LAST. (Issue here.)
Attachment #754741 - Flags: review?(mchen)
Also, there is a crash issue on IPC part so you would need to fix it before landing.
Please see the back trace as below. It is happened when
  1. Play a audio from music app.
  2. Crash will be happened when I perform pause or seek action.

(gdb) bt
#0  0x4147e38e in mozilla::dom::PContentChild::OnMessageReceived (this=0x42e1b618, 
    __msg=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/objdir-gonk-noopt/ipc/ipdl/PContentChild.cpp:2208
#1  0x413628ca in mozilla::ipc::AsyncChannel::OnDispatchMessage (this=0x42e1b624, 
    msg=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/AsyncChannel.cpp:471
#2  0x4136cb10 in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (this=0x42e1b624)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/RPCChannel.cpp:402
#3  0x41334ade in DispatchToMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)()> (obj=0x42e1b624, 
    method=0x4136c959 <mozilla::ipc::RPCChannel::OnMaybeDequeueOne()>, arg=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/tuple.h:383
#4  0x413348b8 in RunnableMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)(), Tuple0>::Run (this=0x42e01ba0)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/task.h:307
#5  0x4136b770 in mozilla::ipc::RPCChannel::RefCountedTask::Run (this=0x42e0a0b8)
    at ../../dist/include/mozilla/ipc/RPCChannel.h:425
#6  0x4136b854 in mozilla::ipc::RPCChannel::DequeueTask::Run (this=0x45ffd250)
---Type <return> to continue, or q <return> to quit---
    at ../../dist/include/mozilla/ipc/RPCChannel.h:448
#7  0x41631ade in MessageLoop::RunTask (this=0xbeffb8bc, task=0x45ffd250)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:337
#8  0x41631b34 in MessageLoop::DeferOrRunPendingTask (this=0xbeffb8bc, 
    pending_task=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:345
#9  0x41631e62 in MessageLoop::DoWork (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:445
#10 0x4136a330 in mozilla::ipc::DoWorkRunnable::Run (this=0x42e033e0)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:42
#11 0x415eaddc in nsThread::ProcessNextEvent (this=0x42e068e0, mayWait=false, 
    result=0xbeffaf47)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/xpcom/threads/nsThread.cpp:620
#12 0x415a4a7a in NS_ProcessNextEvent_P (thread=0x42e068e0, mayWait=false)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/objdir-gonk-noopt/xpcom/build/nsThreadUtils.cpp:237
#13 0x4136a458 in mozilla::ipc::MessagePump::Run (this=0x42e02340, 
---Type <return> to continue, or q <return> to quit---
    aDelegate=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:82
#14 0x4136a7c4 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x42e02340, 
    aDelegate=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:231
#15 0x41631770 in MessageLoop::RunInternal (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:219
#16 0x41631742 in MessageLoop::RunHandler (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:212
#17 0x416316ea in MessageLoop::Run (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:186
#18 0x41260f0e in nsBaseAppShell::Run (this=0x448430a0)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/widget/xpwidgets/nsBaseAppShell.cpp:163
#19 0x40465416 in XRE_RunAppShell ()
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/toolkit/xre/nsEmbedFunctions.cpp:646
#20 0x4136a772 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x42e02340, 
---Type <return> to continue, or q <return> to quit---
    aDelegate=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:198
#21 0x41631770 in MessageLoop::RunInternal (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:219
#22 0x41631742 in MessageLoop::RunHandler (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:212
#23 0x416316ea in MessageLoop::Run (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:186
#24 0x4046504c in XRE_InitChildProcess (aArgc=3, aArgv=0xbeffba44, 
    aProcess=GeckoProcessType_Content)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/toolkit/xre/nsEmbedFunctions.cpp:485
#25 0x00008620 in main (argc=5, argv=0xbeffba44)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/app/MozillaRuntimeMain.cpp:60
> ::: dom/audiochannel/AudioChannelAgentParent.cpp
> @@ +12,5 @@
> > +
> > +void
> > +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  mChildDestroyed = true;
> 
> I think we need to take care the case of child process crashed then
> un-register this agent from AudioChannelService.

When the child destroys the parent, the parent unregister it self in the destructor.
This part is tested. But I can check it better :)

> 1. CanPlay() returns true if this agent is allowed to play.
> 2. But muted variable means if true then this agent should not be allowed to
> play.
> 3. They are opposite meaning here. So after calling SendNotify(!muted)
> agentClient will become "not to play" even agentParent is allowed.

Yeah... I have to rename muted in canPlay and change SendNotify(!muted) to SendNotify(canPlay);

> To image the use case:
>   1. Music app is on playing then alarm is fired.
>   2. Alarm is end then calling UnregisterAudioChannelAgent() ->
> SendAudioChannelChangedNotification() to here.
>   3. Agent with content channel can't be allowed to play because the highest
> channel is still alarm.
>   4. Finally highest channel is remained to initial value -
> AUDIO_CHANNEL_LAST. (Issue here.)

good point.

I'm going to submit a new patch. With the crash fixed!
Attached patch patch (obsolete) — Splinter Review
It took a while. Can you review this code? The crash is fixed and all your comments should be applied.
Attachment #754741 - Attachment is obsolete: true
Attachment #757880 - Flags: review?(mchen)
Attachment #757880 - Flags: review?(bent.mozilla)
Depends on: 862899
Some issues are listed here after doing a simple test.

 Issue 1:
    a. To play an audio from music app.
    b. To play an video from video app.
    c. video and audio are playing concurrently. (NG)
    d. Stop the video then audio is stop too. (wired.)

 Issue 2:
    a. To play an audio from music app.
    b. Set an alarm and wait for it to fire.
    c. Alarm is fired and audio from music app is stopped.
    d. After alarm is end, the audio from music app doesn't be resumed automatically. (NG)

Could you please do the simple tests and make sure the functions are all correct then ask to the review? Thanks.
Attachment #757880 - Flags: review?(mchen)
Comment on attachment 757880 [details] [diff] [review]
patch

Andrea, are you ready for me to review this? Marco had asked for some additional testing before continuing, is that done?
Yes, absolutely. Marco and I are checking each feature, but what I would like to see from you is a review of the IPDL part. In particular the last patch fixes the problem where:

1. clientAgent->StopPlaying(); clientAgent = nullptr;

2. then the parent receives the StopPlaying call and destroys itself. The client has to be kept alive until the communication from the parent is received.
Comment on attachment 757880 [details] [diff] [review]
patch

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

I don't think this is quite ready, we should make some simplifications to the IPDL stuff.

::: dom/audiochannel/AudioChannelAgentChild.h
@@ +14,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class AudioChannelAgentChild : public nsIAudioChannelAgent,
> +                               public AudioChannelAgent,

You should make AudioChannelAgent your first base. Also mark as MOZ_FINAL.

@@ +15,5 @@
> +namespace dom {
> +
> +class AudioChannelAgentChild : public nsIAudioChannelAgent,
> +                               public AudioChannelAgent,
> +                               public PAudioChannelAgentChild

It's really weird that this class is used in both the child and the parent but is called "Child". Can we instead just merge AudioChannelAgentChild into AudioChannelAgent? Then have AudioChannelAgentParent inherit that with protected inheritance or something? That would mean we only have two classes, AudioChannelAgent and AudioChannelAgentParent.

The other alternative is to have AudioChannelAgent be the only class that implements nsISupports and have AudioChannelAgentParent and AudioChannelAgentChild pointers, only one of which is ever used depending on which process you're in.

This current setup is pretty confusing though.

@@ +68,5 @@
> +                        nsIAudioChannelAgentCallback* aCallback,
> +                        bool aUseWeakRef);
> +
> +  virtual void
> +  ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;

Nit: your style is inconsistent here

@@ +74,5 @@
> +  virtual bool StartPlayingInternal() MOZ_OVERRIDE;
> +  virtual void StopPlayingInternal() MOZ_OVERRIDE;
> +  virtual void SetVisibleInternal() MOZ_OVERRIDE;
> +
> +  bool RecvNotify(const bool& aCanPlay) MOZ_OVERRIDE;

Nit: virtual

@@ +80,5 @@
> +  nsWeakPtr mWeakWindow;
> +  nsCOMPtr<nsIAudioChannelAgentCallback> mCallback;
> +  nsWeakPtr mWeakCallback;
> +
> +  nsRefPtr<AudioChannelAgentChild> mKungFu;

This I really don't understand. I think it would be much better to let IPDL "own" this object rather than trying to manage a self-reference in certain circumstances.

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +12,5 @@
> +
> +void
> +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mChildDestroyed = true;

I think rather than tracking this you should just immediately unregister yourself from the service. That way you should die for real and you won't need to check any state below.

@@ +25,5 @@
> +  return service->CanPlay(this);
> +}
> +
> +void
> +AudioChannelAgentParent::StopPlayingInternal(void)

Nit: no need for void

@@ +52,5 @@
> +AudioChannelAgentParent::RecvStopPlaying()
> +{
> +  nsRefPtr<AudioChannelAgentParent> kungFuDeathGrip = this;
> +
> +  if (!mChildDestroyed) {

It shouldn't be possible to get here with mChildDestroyed being true (unless the child sends multiple messages at once, which would be bizarre).

@@ +54,5 @@
> +  nsRefPtr<AudioChannelAgentParent> kungFuDeathGrip = this;
> +
> +  if (!mChildDestroyed) {
> +    Send__delete__(this);
> +    mChildDestroyed = true;

ActorDestroy should have done this already.

::: dom/audiochannel/AudioChannelService.h
@@ +73,5 @@
> +    : mAgent(aAgent)
> +    , mElementHidden(aElementHidden)
> +    {}
> +
> +    virtual ~AudioChannelAgentData()

refcounted classes should have non-public destructors.

::: dom/audiochannel/PAudioChannelAgent.ipdl
@@ +18,5 @@
> +
> +parent:
> +  // When the visibility changes, the AudioChannelService can decide to mute
> +  // this AudioChannelAgent.
> +  sync SetVisible(bool aVisible)

Nit: s/aVisible/visible/

@@ +21,5 @@
> +  // this AudioChannelAgent.
> +  sync SetVisible(bool aVisible)
> +    returns (bool canPlay);
> +
> +  // StopPlaying unregisters the AudioChannelAgentParent and deletes it.

This comment is sorta misleading. It's up to the parent to send the __delete__ message, so you could just say that calling this method triggers a sequence of events that eventually causes the parent to delete the actor?

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +70,3 @@
>     */
> +  void init(in nsIDOMWindow window, in long channelType,
> +            in nsIAudioChannelAgentCallback callback);

Don't forget to bump your iids.

::: dom/ipc/ContentParent.cpp
@@ +88,5 @@
>  #include "StructuredCloneUtils.h"
>  #include "TabParent.h"
>  #include "URIUtils.h"
>  #include "nsGeolocation.h"
> +#include "AudioChannelService.h"

Nit: Alphabetize! (whoever added nsGeolocation gets two demerits).

::: dom/ipc/TabChild.cpp
@@ +1630,5 @@
> +bool
> +TabChild::DeallocPAudioChannelAgent(PAudioChannelAgentChild* actor)
> +{
> +    // Nothing to do here. The child doesn't have to be released when the parent
> +    // is deleted.

Nit: This comment doesn't really make sense, just say "Nothing to do here"

::: dom/ipc/TabParent.cpp
@@ +417,5 @@
> +                                             const AudioChannelType& aType,
> +                                             const bool& aVisible,
> +                                             bool* aCanPlay)
> +{
> +    nsRefPtr<AudioChannelAgentParent> agent =

Nit: No need for the refptr here, you just gave it a ref in Alloc

::: dom/ipc/TabParent.h
@@ +308,5 @@
>      bool mShown;
>      bool mUpdatedDimensions;
>  
>  private:
> +    static uint64_t sTabParentId;

Let's just have this be a global in TabParent.cpp

@@ +337,5 @@
>      // anymore.
>      bool mIsDestroyed;
> +
> +    // A unique ID for this TabParent object
> +    uint64_t mId;

Nit: I'd move this up somewhere before the final bool params.
Attachment #757880 - Flags: review?(bent.mozilla)
Blocks: 823273
Blocks: 855655
No longer blocks: 823273
No longer blocks: 855655
On the plane, flying back to europe I had time to play a bit with this patch.
This patch introduces the window in any Init() method of nsIAudioChannelAgent.

This is the first patch of 3.
Attachment #757880 - Attachment is obsolete: true
Attachment #826784 - Flags: review?(mchen)
I decided to work on this issue just to see in practice what I and bent discussed during the IPDL "work week".
This patch does this:

1. PAudioChannelAgent.ipdl: a simple protocol

2. AudioChannelAgent is also a PAudioChannelAgentParent.

3. When AudioChannelAgent runs on a child process, it creates a AudioChannelAgentChild and sends all the request through the PAudioChannelAgent protocol.

I wrote a test to check if this works.
Attachment #826788 - Flags: review?(mchen)
Attachment #826788 - Flags: review?(bent.mozilla)
Comment on attachment 826784 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +72,3 @@
>  }
>  
> +/* void initWithVideo(in nsIDOMWindow window, in long channelType,

this is moved before line 66.
The last patch should clean a bit the AudioChannelService code but I don't have time to work on it.
Small issues fixed.
Attachment #826788 - Attachment is obsolete: true
Attachment #826788 - Flags: review?(mchen)
Attachment #826788 - Flags: review?(bent.mozilla)
Attachment #827055 - Flags: review?(mchen)
Attachment #827055 - Flags: review?(bent.mozilla)
Comment on attachment 827055 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

Hi Andrea,

I tested your patch on master branch but the behavior of audio competing became not normal.

  1. lunch music player and play it.
  2. lunch video or fm and play it.
  3. both music and video/fm are playing
  4. stop video/fm then music is stopped too.

May I know your testing result or I missed to apply something?

::: content/html/content/src/HTMLAudioElement.cpp
@@ +272,5 @@
>      HTMLMediaElement::UpdateAudioChannelPlayingState();
>      return;
>    }
> +
> +  if (UseAudioChannelService() &&

We can move "the check of UseAudioChannelService()" before here then we can save one level inside if-else.

if (!UseAudioChannelService()) {
  return;
}
Attachment #827055 - Flags: review?(mchen)
> I didn't complete the review but there are some defect found.

Yes. It's normal! These are the first 2 patches of 3. With the 3rd one the behaviour will be the same.
I hope to have time to work on the last patch.

Thanks for the comments, I'll check them soon.
Attachment #826784 - Attachment is obsolete: true
Attachment #826784 - Flags: review?(mchen)
Attachment #829701 - Flags: review?(mchen)
Attachment #827055 - Attachment is obsolete: true
Attachment #827055 - Flags: review?(bent.mozilla)
Attachment #829702 - Flags: review?(bent.mozilla)
Attachment #829703 - Flags: review?(mchen)
These 3 patches are green on try and they work on a real device. Of course some regression can be found.
Marco, from you I would like to know if you like this code more than the previous one. I think it's easier to understand and to be changed. Then reviews are welcome!

Something nice of this code is the groupID object. With this we group per window/tab/app. I think this code can be reused in the SpeakerManager too and probably somewhere else.
A follow up, when this code is landed, is to add a permission check in the AudioChannelAgentParent so that we can see if the app has the right permission to use a particular AudioChannelType.
(In reply to Andrea Marchesini (:baku) from comment #25)
> A follow up, when this code is landed, is to add a permission check in the
> AudioChannelAgentParent so that we can see if the app has the right
> permission to use a particular AudioChannelType.

About this improvement, do we need to check permission in AudioChannelAgentParent?
The only way now for assigning audio channel explicitly is by argument/property from video/audio element.
And we already did permission check there.
> About this improvement, do we need to check permission in
> AudioChannelAgentParent?
> The only way now for assigning audio channel explicitly is by
> argument/property from video/audio element.
> And we already did permission check there.

What about if the child process has been hacked? A MediaElement can skip that check and play without additional permission checks. One more is simple and nice to have. Of course... if all the rest of these patches are good enough :)
(In reply to Andrea Marchesini (:baku) from comment #27)
> What about if the child process has been hacked? 
This is not a normal case and we should fixed the weakness been attacked not build a second wall there.

And I am going to review and do some simple test.
Comment on attachment 829701 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

It looks good except the one question about window in DOMCameraControl.

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +5,5 @@
>  #include "AudioChannelAgent.h"
>  #include "AudioChannelCommon.h"
>  #include "AudioChannelService.h"
> +#include "nsIDOMWindow.h"
> +#include "nsXULAppAPI.h"

Do we need this header?

@@ +71,3 @@
>                                   bool aUseWeakRef)
>  {
> +  return InitInternal(aWindow, aChannelType, aCallback, aUseWeakRef, true);

Could you help to add "/* useWeakRef = */ true"?
Thanks.

::: dom/camera/DOMCameraControl.cpp
@@ +374,5 @@
>    if (!mAudioChannelAgent) {
>      mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
>      if (mAudioChannelAgent) {
>        // Camera app will stop recording when it falls to the background, so no callback is necessary.
> +      mAudioChannelAgent->Init(nullptr, AUDIO_CHANNEL_CONTENT, nullptr);

This object is running on child process so it needs a window (mWindow).
Or it will hit the assertion in AudioChannelAgent.cpp
Attachment #829701 - Flags: review?(mchen)
Comment on attachment 829702 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

This mostly looks fine, but r- for the protocol error in PAudioChannelAgent.ipdl

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +75,5 @@
>                                   nsIAudioChannelAgentCallback *aCallback,
>                                   bool aUseWeakRef)
>  {
> +  return InitInternal(aWindow, aChannelType, aCallback, aUseWeakRef,
> +                      /* useWeakRef = */ true);

Wait, there are two args that specify whether to use weak refs?

@@ +126,5 @@
>  }
>  
>  /* boolean startPlaying (); */
> +NS_IMETHODIMP
> +AudioChannelAgent::StartPlaying(int32_t *aCanPlay)

Nit: * are sometimes on the left, sometimes on the right...

@@ +148,5 @@
> +
> +    nsCOMPtr<nsPIDOMWindow> window = static_cast<nsPIDOMWindow*>(mWindow.get());
> +    MOZ_ASSERT(window);
> +
> +    TabChild* tabChild =  dom::TabChild::GetFrom(window->GetDocShell());

Nit: No need for the dom:: here.

@@ +158,5 @@
> +             static_cast<AudioChannelType>(mAudioChannelType),
> +             mVisible, aCanPlay);
> +    MOZ_ASSERT(mIPC);
> +
> +    static_cast<AudioChannelAgentChild*>(mIPC)->SetParent(this);

This would be better with the other constructor:

  AudioChannelAgentChild* actor = new AudioChannelAgentChild(this);
  tabChild->SendPAudioChannelAgentConstructor(actor, ...);

It would mean adding a non-default constructor to AudioChannelAgentChild but that's ok.

@@ +182,5 @@
>    mIsRegToService = false;
> +
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    if (!mIPC) {
> +      return NS_OK;

This can be cleaned up as:

  if (XRE_GetProcessType() != GeckoProcessType_Default && mIPC) {

There's another spot below too.

@@ +194,5 @@
>  }
>  
>  /* void setVisibilityState (in boolean visible); */
> +NS_IMETHODIMP
> +AudioChannelAgent::SetVisibilityState(bool visible)

Nit: aVisible

::: dom/audiochannel/AudioChannelAgent.h
@@ +8,5 @@
>  #define mozilla_dom_audio_channel_agent_h__
>  
>  #include "nsIAudioChannelAgent.h"
>  #include "nsCycleCollectionParticipant.h"
> +#include "mozilla/dom/PAudioChannelAgentChild.h"

No need to include this here, just forward-declare.

@@ +28,1 @@
>  /* Header file */

Nit: remove

@@ +50,5 @@
>                          nsIAudioChannelAgentCallback* aCallback,
>                          bool aUseWeakRef, bool aWithVideo=false);
>  
>    nsCOMPtr<nsIDOMWindow> mWindow;
> +  PAudioChannelAgentChild* mIPC;

Nit: mActor is much more readable

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +7,5 @@
> +#include "mozilla/dom/TabParent.h"
> +
> +using namespace mozilla::dom;
> +
> +NS_IMPL_ADDREF_INHERITED(AudioChannelAgentParent, AudioChannelAgent)

All of this macro goop can be replaces with:

  NS_IMPL_ISUPPORTS_INHERITED0(AudioChannelAgentParent, AudioChannelAgent)

::: dom/audiochannel/AudioChannelAgentParent.h
@@ +14,5 @@
> +namespace dom {
> +
> +class TabParent;
> +
> +/* Header file */

Nit: remove

@@ +21,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelAgentParent,
> +                                           AudioChannelAgent)

I don't think you need to add anything to the CC map, no need for this macro.

@@ +23,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelAgentParent,
> +                                           AudioChannelAgent)
> +
> +  AudioChannelAgentParent()

This and the destructor should be moved to the cpp file, otherwise the compiler can't figure out what to do with the forward-declared TabParent.

@@ +37,5 @@
> +  RecvSetVisibleState(const bool& aVisible,
> +                      int32_t* aCanPlay) MOZ_OVERRIDE;
> +
> +  virtual bool
> +  Recv__delete__();

Nit: MOZ_OVERRIDE

::: dom/audiochannel/AudioChannelService.cpp
@@ +151,4 @@
>  
> +  // In order to avoid race conditions, it's safer to notify any existing
> +  // agent any time a new one is registered.
> +  SendAudioChannelChangedNotification(aChildID);

Wait, you removed this message from PContent... What does this do now?

::: dom/audiochannel/PAudioChannelAgent.ipdl
@@ +26,5 @@
> +child:
> +  // Notify is called when the AudioChannelService wants to mute this
> +  // AudioChannelAgent. Read nsIAudioChannelAgent.idl for more info about
> +  // canPlay values.
> +  Notify(int32_t aCanPlay);

There's nothing here to prevent sending a Notify() from parent->child at the same time as sending the __delete__ from child->parent. You have to prevent that or the parent could crash.

::: dom/ipc/TabParent.cpp
@@ +1336,5 @@
> +                                             const bool& aVisible,
> +                                             int32_t* aCanPlay)
> +{
> +  nsRefPtr<AudioChannelAgentParent> agent =
> +    static_cast<AudioChannelAgentParent*>(aActor);

Nit: You know there's a ref here, no need to add another

@@ +1339,5 @@
> +  nsRefPtr<AudioChannelAgentParent> agent =
> +    static_cast<AudioChannelAgentParent*>(aActor);
> +
> +  nsresult rv = agent->InitFromTabParent(this, aType);
> +  NS_ENSURE_SUCCESS(rv, nullptr);

Eek! This method returns bool :)

@@ +1345,5 @@
> +  rv = agent->SetVisibilityState(aVisible);
> +  NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +  agent->StartPlaying(aCanPlay);
> +  NS_ENSURE_SUCCESS(rv, nullptr);

Shouldn't you be setting rv from StartPlaying()?

::: ipc/glue/IPCMessageUtils.h
@@ +19,5 @@
>  #include <stdint.h>
>  
>  #include "nsID.h"
>  #include "nsMemory.h"
> +#include "nsStringGlue.h"

Did you need to do this? It's rather unlikely that IPCMessageUtils can work outside of libxul
Attachment #829702 - Flags: review?(bent.mozilla) → review-
I cannot remove nsXULAppAPI.h because it's needed for XRE_GetProcessType()
Attachment #829701 - Attachment is obsolete: true
Attachment #8335337 - Flags: review?(mchen)
> @@ +23,5 @@
> > +  NS_DECL_ISUPPORTS_INHERITED
> > +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelAgentParent,
> > +                                           AudioChannelAgent)
> > +
> > +  AudioChannelAgentParent()
> 
> This and the destructor should be moved to the cpp file, otherwise the
> compiler can't figure out what to do with the forward-declared TabParent.

it actually works. why do you say this?

> Wait, you removed this message from PContent... What does this do now?

Now we use the RecvNotify() in the AudioChannelAgentChild.

> There's nothing here to prevent sending a Notify() from parent->child at the
> same time as sending the __delete__ from child->parent. You have to prevent
> that or the parent could crash.

Still working on this.

> Did you need to do this? It's rather unlikely that IPCMessageUtils can work
> outside of libxul

This is needed for a CPP Unit Test that runs outside of libxul.
Comment on attachment 829702 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +140,5 @@
>    }
>  
>    service->RegisterAudioChannelAgent(this,
>      static_cast<AudioChannelType>(mAudioChannelType), mWithVideo);
> +  mIsRegToService = true;

Line 167 also assigned mIsRegToService to true.
Maybe we need to move this line before line 163.

::: dom/ipc/PBrowser.ipdl
@@ +319,5 @@
>  
> +    /**
> +     * Create a new AudioChannelAgent.
> +     */
> +    sync PAudioChannelAgent(AudioChannelType aType, bool aVisible)

There is a new attribute called AudioChannelAgent::mWithVideo.
This is used to notify AudioChannelService whether this audio is a from a video or not.
We need to send mWithVideo to parent side too.
Comment on attachment 829703 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

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

Hi,

This change looks good to me and actually improve the readable of the codes. Great Job.

But there are some bugs and questions in this patch, please help to address them.
Thanks.

::: content/html/content/src/HTMLAudioElement.cpp
@@ +283,5 @@
>      if (!mAudioChannelAgent) {
> +      nsCOMPtr<nsIDOMWindow> window = OwnerDoc()->GetWindow();
> +      if (!window) {
> +        return;
> +      }

NS_ENSURE_TRUE_VOID(window);

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3857,5 @@
>      if (!mAudioChannelAgent) {
> +      nsCOMPtr<nsIDOMWindow> window = OwnerDoc()->GetWindow();
> +      if (!window) {
> +        return;
> +      }

NS_ENSURE_TRUE_VOID(window);

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +185,5 @@
>    if (!service) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  service->RegisterAudioChannelAgent(this);

Since there is a representative (AudioChannelAgentParent) in parent process of each AudioChannelAgent in child process. In child process, do we still need to register each AudioChannelAgent into AudioChannelServiceClient.

It seems there is no necessary of AudioChannelServiceClient to know and own AudioChannelAgents.

::: dom/audiochannel/AudioChannelCommon.h
@@ +44,5 @@
> +    Unknown = 0,
> +    App,
> +    BrowserTab,
> +    Window
> +  } mType;

Does mType need to be public or just private?

@@ +96,5 @@
> +  }
> +
> +private:
> +  uint64_t mId;
> +  uint64_t mProcessId;

I only saw one line to do assignment into mProcessId and the value is CONTENT_PROCESS_ID_MAIN.
And it seems to be no meaning of comparing mProcessId in operator == or !=.

If it is useful then could we call it mChildId (I am confused between mProcessId and pid of a process)?

::: dom/audiochannel/AudioChannelService.cpp
@@ +259,5 @@
> +      // updated properly as hidden, mPlayableHiddenContentChildID is used to
> +      // check whether this background process is playable or not.
> +      CountAgents(AUDIO_CHANNEL_CONTENT, Visible,
> +                  aAgent->GroupId().ProcessId() == 0 &&
> +      mPlayableHiddenContentGroupId != aAgent->GroupId())) {

The right parenthesis of CountAgents is wrong.

@@ +382,5 @@
> +    if (aAgent->Type() >= AUDIO_CHANNEL_NOTIFICATION) {
> +      data->mHigherChannel = aAgent->Type();
> +    }
> +    else if (aAgent->Type() == AUDIO_CHANNEL_CONTENT) {
> +      if (gAudioChannelService->mPlayableHiddenContentGroupId.IsUnknown()) {

Consider the case as below.
  1. An App is starting to play a music and be put into background.
  2. Now the mPlayableHiddenContentGroupId will not be an unknown ID so mHigherChannel didn't be updated.
This will let HW volume keys not control content channel.

Maybe we can 
  1. change mPlayableHiddenContentGroupId to mPlayableContentGroupId.
  2. at line 141/228, assign agent's groupId into mPlayableContentGroupId.
  3. then we can avoid to count in muted agent too.

@@ +493,5 @@
>    }
>  
> +  return PL_DHASH_NEXT;
> +}
> +  

remove spaces.
Attachment #829703 - Flags: review?(mchen)
Comment on attachment 8335337 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

Hi Andrea,

It looks good to me for all the changes here, but how about the AudioChannelAgent in FMRadio.
Could you help to add that part? Or this patch can't be landed.
Attachment #8335337 - Flags: review?(mchen)
> > +  uint64_t mProcessId;
> 
> I only saw one line to do assignment into mProcessId and the value is
> CONTENT_PROCESS_ID_MAIN.
> And it seems to be no meaning of comparing mProcessId in operator == or !=.

Actually mProcessId (now renamed mChildID) is setwhen  AudioChannelGroupId is created from a TabParent.
This is the reason why I need to compare it too.

> @@ +382,5 @@
> > +    if (aAgent->Type() >= AUDIO_CHANNEL_NOTIFICATION) {
> > +      data->mHigherChannel = aAgent->Type();
> > +    }
> > +    else if (aAgent->Type() == AUDIO_CHANNEL_CONTENT) {
> > +      if (gAudioChannelService->mPlayableHiddenContentGroupId.IsUnknown()) {
> 
> Consider the case as below.
>   1. An App is starting to play a music and be put into background.
>   2. Now the mPlayableHiddenContentGroupId will not be an unknown ID so
> mHigherChannel didn't be updated.
> This will let HW volume keys not control content channel.

I'll work on this.
> Consider the case as below.
>   1. An App is starting to play a music and be put into background.
>   2. Now the mPlayableHiddenContentGroupId will not be an unknown ID so
> mHigherChannel didn't be updated.
> This will let HW volume keys not control content channel.

I don't follow. Actually I think that the code can be written in this way:

PLDHashOperator
AudioChannelService::HigherChannelsEnumerator(AudioChannelAgent* aAgent,                                  
                                              AudioChannelAgentData* aData,
                                              void *aPtr)                                                 
{   
  HigherChannelsData* data = static_cast<HigherChannelsData*>(aPtr);                                      
  
  // Visible agents                                                                                       
  if (data->mVisibleHigherChannel < aAgent->Type() && aAgent->VisibleState()) {                           
    data->mVisibleHigherChannel = aAgent->Type(); 
  }                                                                                                       
  
  // Any agents
  if (data->mHigherChannel < aAgent->Type()) {                                                            
    data->mHigherChannel = aAgent->Type();                                                                
  } 
      
  return PL_DHASH_NEXT;                                                                                   
}     
    

what do you think?
Flags: needinfo?(mchen)
(In reply to Andrea Marchesini (:baku) from comment #37)
> what do you think?

Consider the use case as below:

  1. Play a audio from music app.
  2. Put music app into background.
  3. Play then stop video from video app in the foreground.

Then in this moment, the mHigherChannel will be "content" channel but actually that audio from music app is paused by AudioChannel already. Then in homescreen, HW key will control normal/content not notification/ringer channel.
Flags: needinfo?(mchen)
>   1. Play a audio from music app.
>   2. Put music app into background.
>   3. Play then stop video from video app in the foreground.

In this case we don't want to restore the audio of the music app. So probably we should set mPlayableHiddenContentGroupId to unknown. Sort of:

if (aAgent->WithVideo() && aAgent->VisibleState()) {
  mPlayableHiddenContentGroupId = AudioChannelGroupId::UnknownGroupId();
}
> else if (aAgent->Type() == AUDIO_CHANNEL_CONTENT && aAgent->VisibleState()) {
> mPlayableHiddenContentGroupId = AudioChannelGroupId::UnknownGroupId();
> }

In patch 3's AudioChannelService::RegisterAudioChannelAgent(...), codes above will clear mPlayableHiddenContentGroupId already. So in AudioChannelService::HigherChannelsEnumerator() we should do check for "mHigherChannel"

  if (data->mHigherChannel < aAgent->Type()) {
    // If this agent is a content channel in the background and it was paused by policy then we should not count it in.
    if (!aAgent->VisibleState() && aAgent->Type() == AUDIO_CHANNEL_CONTENT && gAudioChannelService->mPlayableHiddenContentGroupId.IsUnknown()) {
      return PL_DHASH_NEXT;
    }
    data->mHigherChannel = aAgent->Type();                                                                
  }
Marco, thanks for your help.
Attachment #829703 - Attachment is obsolete: true
Attachment #8343053 - Flags: review?(mchen)
Attachment #829702 - Attachment is obsolete: true
Attachment #8343230 - Flags: review?(bent.mozilla)
Comment on attachment 8343053 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

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

The other parts are great to me and expect to see it's landing. Thanks.

::: dom/audiochannel/AudioChannelService.cpp
@@ +469,2 @@
>    mDeferTelChannelTimer = nullptr;
> +  mDeferTelChannelAgent = nullptr;

Currently dialer app has a telephony channel for sending key tone during voice_call. When incoming call is coming (dialer is not launched yet) and remote side close the call, the sequences as below are happened

1. dialer app is killed because incoming call is dropped by remote side.
2. GC deletes AudioChannelAgentParent for telephony channel from dialer app.
3. The deconstructor of AudioChannelAgentParent calls stopPlaying() and finally go into the code at line 168 (mDeferTelChannelAgent = aAgent).
4. After timer is expired, the function here is called and assign mDeferTelChannelAgent to null.

According to this agent is already freed during step 2, release() operation performed by smart pointer will be crashed.

I didn't figure out the solution and the first idea is 
  1. add/remove reference actively for audiochannelagent when put/remove it into/from mAgent.
  2. In processing of ipc-shutdown from ::observer(), before remove agent from mAgent we can help to call ::UnregisterAudioChannelAgent() first.
Attachment #8343053 - Flags: review?(mchen)
the hashtable can take care of the refcounting object if we use nsRefPtrHashKey.
Is this enough? I also added a security check if the child is destroyed before the parent.
Attachment #8343053 - Attachment is obsolete: true
Attachment #8355216 - Flags: review?(mchen)
Comment on attachment 8355216 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

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

I think it is really close to be landed.
And how about the patch for AudioChannelAgent in FMRadio Client?

Thanks.

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +41,5 @@
>  bool
>  AudioChannelAgentParent::RecvSetVisibleState(const bool& aVisible,
>                                               int32_t* aCanPlay)
>  {
> +  MOZ_ASSERT(!mDestroyed);

Since "ActorDestroy()" was called already, does it still have chance to receive "SetVisibleState" and "StopPlaying"?
I think these two are redundant.
Attachment #8355216 - Flags: review?(mchen)
> I think it is really close to be landed.
> And how about the patch for AudioChannelAgent in FMRadio Client?

That landed. did it?

> Since "ActorDestroy()" was called already, does it still have chance to
> receive "SetVisibleState" and "StopPlaying"?
> I think these two are redundant.

I agree with you but it's just for debugging build and it's a nice check to have.
Hi Andrea,

I mean there is a new attribute - window which should be added into each initialization of AudioChannelAgent (child side).

http://hg.mozilla.org/mozilla-central/file/49d2fce9a86c/dom/fmradio/FMRadio.cpp#l152
patch updated.
Attachment #8335337 - Attachment is obsolete: true
Attachment #8355499 - Flags: review?(mchen)
Comment on attachment 8355499 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

And please move the modification of window attribute in HTMLAudioElement from patch 2 to here. 

Thanks.

::: dom/fmradio/FMRadio.cpp
@@ +149,5 @@
>      do_CreateInstance("@mozilla.org/audiochannelagent;1");
>    NS_ENSURE_TRUE_VOID(audioChannelAgent);
>  
>    audioChannelAgent->InitWithWeakCallback(
> +    static_cast<nsIDOMWindow*>(GetOwner()),

Why do you need to add explicitly casting here but didn't apply in other places?
Attachment #8355499 - Flags: review?(mchen)
I just changed what you said in your comments.
Attachment #8355499 - Attachment is obsolete: true
Attachment #8358352 - Flags: review?(mchen)
Attachment #8343230 - Attachment is obsolete: true
Attachment #8343230 - Flags: review?(bent.mozilla)
Attachment #8358353 - Flags: review?(bent.mozilla)
So, it seems that we are almost there. We need just an additional review for the IPDL part.
mchen, what do you think about landing this piece of code soon? Do we have to wait for a particular release?
Flags: needinfo?(mchen)
Attachment #8358352 - Flags: review?(mchen) → review+
(In reply to Andrea Marchesini (:baku) from comment #52)
> So, it seems that we are almost there. We need just an additional review for
> the IPDL part.
> mchen, what do you think about landing this piece of code soon? Do we have
> to wait for a particular release?

I think now is a right timing because V1.3 already is branched into aurora and V1.4 on master didn't start the sprint 1 yet. So we should not break any mile stone if there is a regression there unfortunately.
Flags: needinfo?(mchen)
Comment on attachment 8358353 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

Blake, do you mind stealing this from Ben please?
Attachment #8358353 - Flags: review?(mrbkap)
I think it's impossible to have these patches landed before bug 1016277 is fixed. Right?
I'm going to get feedback here by the end of the week.
Requesting this to be prioritized into 2.2 since this blocks bug 876631.
See bug 1015073 for user-facing issue.
feature-b2g: --- → 2.2?
Comment on attachment 8358353 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

I think bent needs to review this. I have a couple of questions, though.

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +31,5 @@
>    , mWithVideo(false)
>  {
>  }
>  
>  AudioChannelAgent::~AudioChannelAgent()

Do we have to worry about the shutdown case here, where the connection to the parent is lost and we get dealloc'd by the manager?

@@ +203,3 @@
>    nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback();
>  
> +  if (XRE_GetProcessType() != GeckoProcessType_Default && mActor) {

Why does it make sense to call into the service if we're in a child process but mActor is null?

::: dom/ipc/ContentChild.cpp
@@ +119,5 @@
>  #include "nsContentUtils.h"
>  #include "nsIPrincipal.h"
>  #include "nsDeviceStorage.h"
>  #include "AudioChannelService.h"
> +#include "AudioChannelService.h"

Duplicated.

::: dom/ipc/TabParent.cpp
@@ +1432,5 @@
> +                                         const bool& aWithVideo,
> +                                         int32_t* aCanPlay)
> +{
> +  nsRefPtr<AudioChannelAgentParent> agent = new AudioChannelAgentParent();
> +  return agent.forget().get();

Doesn't this have to be |take()| instead of |get()| now?
Attachment #8358353 - Flags: review?(mrbkap)
> ::: dom/audiochannel/AudioChannelAgent.cpp
> @@ +31,5 @@
> >    , mWithVideo(false)
> >  {
> >  }
> >  
> >  AudioChannelAgent::~AudioChannelAgent()
> 
> Do we have to worry about the shutdown case here, where the connection to
> the parent is lost and we get dealloc'd by the manager?

In this case the parent should receive an ActorDestoy() call and magically that should be handled.


> > +  if (XRE_GetProcessType() != GeckoProcessType_Default && mActor) {
> 
> Why does it make sense to call into the service if we're in a child process
> but mActor is null?

I don't remember exactly all this code but I know that, the AudioChannelService in the child process is used to know if there is something active in the current app.

> > +  nsRefPtr<AudioChannelAgentParent> agent = new AudioChannelAgentParent();
> > +  return agent.forget().get();
> 
> Doesn't this have to be |take()| instead of |get()| now?

At that time it was take() :)
I have to rewrit^H^H^Hrebase this patch and I guess I'll find a lot of these issue.
Bobby, please follow up this topic.
feature-b2g: 2.2? → ---
Flags: needinfo?(bchien)
I'm working on the new AudioChannel API.
I guess we can close this bug and file a new one for the new API.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
No longer blocks: 853101
clear ni. Already link to AudioChannel meta.
Flags: needinfo?(bchien)
Attachment #8358353 - Attachment is obsolete: true
Attachment #8358353 - Flags: review?(bent.mozilla)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: