845365 - audio-channel-changed is not properly emitted when "content" is not playing

Status: RESOLVED WONTFIX

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Marco Chen [:mchen]]

Comment on attachment 718454 [details] [diff] [review]
patch

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

Hi Andrea,

What to do in thia patch is covered by Bug 836655 which is reviewed by you.
Or you think you want to modify that part by this patch.

If yes, could you rebase i so I can make sure you know the changed from 836655 already?

Thanks.

Comment 3

[Andrea Marchesini [:baku]]

> What to do in thia patch is covered by Bug 836655 which is reviewed by you.
> Or you think you want to modify that part by this patch.

Yeah... I'm a bit confused :) your changes is good enough. The only different is that here I do a check for each element. Probably it's better... in some rare corner case.
I'll rebase the patch and review it low-priority. Thanks.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

Here the patch rebased. It's low profile, but here we check any element of the array instead just the first one.

Comment 5

[Marco Chen [:mchen]]

From the code now for adding & removing childID into active list, is any condition there are more then two child ID in the list when checking on the case of background? Thanks.

Comment 6

[Andrea Marchesini [:baku]]

right now no. But it's possible to have 2 visible apps: keyboard and something else.

Comment 7

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #6)
>But it's possible to have 2 visible apps: keyboard and
> something else.

The case of two apps you mentioned is for foreground state but the patch here is for checking in the background state. Since code already limited just only one can be kept in the background state, why do we need to check more then index - 0?

Comment 8

[Andrea Marchesini [:baku]]

> The case of two apps you mentioned is for foreground state but the patch
> here is for checking in the background state. Since code already limited
> just only one can be kept in the background state, why do we need to check
> more then index - 0?

The code doesn't limit the number of apps in background. Any active visible app, is saved in the mActiveContentChildIDs array. If you have 2 active apps, then you send both to foreground, the mActiveContentChildIDs will contain 2 apps IDs (childIDs).

if one of these 2 apps stops playing, the service doesn't remove the chilID from mActiveContentChildIDs (because maybe this is just a short stop/start sequence).

-    else if (!mActiveContentChildIDs.IsEmpty() &&
-             mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].Contains(
-             mActiveContentChildIDs[0])) {
-      higher = AUDIO_CHANNEL_CONTENT;

So maybe mActiveContentChildIDs[0] is not contained by mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN] because that app stopped playing.

Just [1] is still in mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].

Comment 9

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #8)
> if one of these 2 apps stops playing, the service doesn't remove the chilID
> from mActiveContentChildIDs (because maybe this is just a short stop/start
> sequence).
> 

Pleaser refer to the code link as below. The childID in foreground will be removed from list when it stops to play. 
http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#139

And the link below shows that the childID who is moving to background will be removed from list if there is still a content channel playing in the foreground but not belong to it.
http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#220

So finally if there is no content channel in the foreground then only one childID will be kept in the list.
http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#220

Comment 10

[Marco Chen [:mchen]]

Comment on attachment 719048 [details] [diff] [review]
patch

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

Please refer to comment 9 for why I change the review flag to null.
Thanks.

Comment 11

[Andrea Marchesini [:baku]]

We are working on a new AudioChannel API. This bug is not a need.