1133449 - [B2G] The default audio type didn't be set correctly when the call screen app is launched

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Alastor Wu [:alwu]]

This bug would open for the regression bug in regression. 
https://bugzilla.mozilla.org/show_bug.cgi?id=1092346#c15

After re-study the regression error, I found that the patch of the Bug1092346 is not a good solution. The real root cause is that the wrong visible state of the call screen app. 

In the AudioChannelService, we check the visible state to decide who can set the default audio channel type. The foreground app can always set the type. 

BUT! This visible state is the active state of the docshell. The call screen app is running on the main process, so its visible state would always be true. So the following thing would happen.

STR
1. Revert the changeset of Bug1092346

2. Open X app (any apps)
   - foreground : X app 

3. Incoming call or make a call 
   - foreground : call screen
   - background : X app

4. Hang up the call 
   - foreground : X app & call screen

In the last step, X app set its default audio type first, then the call screen also did that . That means the default type would always be changed by call screen.

Comment 1

[Alastor Wu [:alwu]]

Attachment: don't use the visible state of the app in parent process

Hi, Baku,
Sorry to bother you again,
You reviewed my previous patch in the Bug1092346, could you help me again :)? 
Please~~~

Because the visible state of the app from parent process is not correct, I use the |mDefChannelChildID| to decide whether other app is using the default audio channel. 

Very very appreciate!!!

Comment 2

[Alastor Wu [:alwu]]

Attachment: rename the parameter

The input parameters of |SetDefaultVolumeControlChannelInternal()| are sent from the AudioChannelManager::NotifyVolumeControlChannelChanged().

The second parameter is |isActive|, but we use |aHidden| to receive it. It doesn't make sense. So I renamed the parameter.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8564940 [details] [diff] [review]
don't use the visible state of the app in parent process

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +543,4 @@
>      return;
>    }
>  
> +  mDefChannelChildID = (aVisible)? aChildID : CONTENT_PROCESS_ID_UNKNOWN;

aVisible ? aChildID : CONTENT_PROCESS_ID_UNKNOWN;

@@ +547,1 @@
>    nsString channelName;

can you change this to: nsAutoString ?

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Alastor Wu [:alwu] from comment #0)
> BUT! This visible state is the active state of the docshell. The call screen
> app is running on the main process, so its visible state would always be
> true.

Thanks for looking into this! I had figured out in bug 1092346 that this was somehow due to a visibility issue but I thought it was due to the order in which visibilitychange events were delivered. This is at least the third hack I know of that's caused by the callscreen living in the main process. I hope we'll be able to split it out soon; the only reason why it was there was to avoid slow starts in memory-constrained devices (tarako) but now that we have cgroup support we should be able to use that to solve the problem and pull out the callscreen again.

Comment 5

[Alastor Wu [:alwu]]

Attachment: don't use the visible state of the app in parent process. r=baku.

Comment 6

[Alastor Wu [:alwu]]

Attachment: rename the parameter. r=baku.

Comment 7

[Alastor Wu [:alwu]]

Try-server result.
https://tbpl.mozilla.org/?tree=Try&rev=24dde3277b95

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 9

[Alastor Wu [:alwu]]

Attachment: rename the parameter. r=baku.

Comment 10

[Alastor Wu [:alwu]]

Attachment: don't use the visible state of the app in parent process. r=baku.

Comment 11

[Alastor Wu [:alwu]]

Sorry for my mistake,
Here is the new try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17d702112f9b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c43b96ec5497
https://hg.mozilla.org/integration/mozilla-inbound/rev/46feaa843a2a

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c43b96ec5497
https://hg.mozilla.org/mozilla-central/rev/46feaa843a2a