Remove the dependency on apps status in the audiochannel code

VERIFIED FIXED

Status

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: fabrice, Assigned: gsvelto)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
It should be chrome only now. See http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioChannelManager.cpp#157 for a starting point of things to fix.
(Assignee)

Comment 1

3 years ago
I've started hacking on this.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
As we imagined this is a can of worms, some of the things that will need fixing:

- Various apps use the audio-channel-* permissions to adjust the volume, breaking this is not particularly bad and in fact we could replace it with popup permissions on first use (e.g. do you want to let the clock app adjust the alarm audio channel?). This is the simplest issue to address IMHO

- Create audio contexts for a specific channel. This is trickier to solve but it's mostly used for things like telephony, the ringer or the alarm channels.

- Handle interruptions (e.g. a channel gets switched out because a higher priority one starts playing, for example the ringtone going off while the music is playing). This is the trickiest one because it uses app manifests to identify which app is using what channel.

One way to fix all the above in one fell swoop (while losing some functionality) would be the following:

- Let chrome apps access any channel they like, this covers many cases because the system app is the biggest user of this API. It would also allow us to deploy workarounds for things like the ringer, alarm and notification (i.e. those would be delegated to the system app)

- Let content apps use only the content channel. Note that in many cases we use the API only for using it which would make these uses redundant.

- Adapt interrupt support to keep working without permissions. I'm not sure if this is feasible but it should preserve most of the functionality we need (pausing video/audio when something else is playing)
(Assignee)

Comment 3

3 years ago
Created attachment 8728978 [details] [diff] [review]
[PATCH WIP] Remove dependencies to the mozApps-related APIs from the audio channel code

This is the first version of the patch that builds correctly. I've had to rip out the NotifyChannel functionality which had been introduced very recently and was sadly never used (and never will since it relies on the apps service and system messages). Interrupt start/end functionality is still intact though but that's not going to be for long too because it also relies on system messages. We might consider removing that too. It's a handy feature for the media apps to interact nicely with incoming calls but it's not a hard requirement for having a working phone.
(Assignee)

Comment 4

3 years ago
Created attachment 8728990 [details] [diff] [review]
[PATCH WIP v2] Remove dependencies to the mozApps-related APIs from the audio channel code

Updated patch ripping out the affected tests and some related code too. As soon as we have the system app running as chrome this should be easy to test. We should cook up a patch to remove the audio-channel-related permissions from the various gaia apps and lump everybody on the content channel. We can always fix things up later if we decide to move the permissions into regular web manifests or if we use pop-ups or some other alternative to FxOS manifests.
Attachment #8728978 - Attachment is obsolete: true
Attachment #8728990 - Flags: feedback?(fabrice)
Comment on attachment 8728990 [details] [diff] [review]
[PATCH WIP v2] Remove dependencies to the mozApps-related APIs from the audio channel code

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +110,5 @@
>      return NS_OK;
>    }
>  
> +  if (nsContentUtils::IsChromeDoc(doc)) {
> +    return NS_OK; // System app

If we can check whether the content is system app without using the pref "b2g.system_manifest_url", you can should remove the pref in other audio-channel related tests. 
ex: browserElement_AudioChannel.js, browserElement_AudioPlayback.js .. e.t.c.

::: dom/browser-element/mochitest/chrome.ini
@@ -11,5 @@
> -  manifest.webapp^headers^
> -  multipleAudioChannels_manifest.webapp
> -  multipleAudioChannels_manifest.webapp^headers^
> -
> -[test_browserElement_MultipleAudioChannels.html]

Don't remove this test and related files, they are not related with NotifyChannel.
(Assignee)

Comment 6

3 years ago
Nice catch, it seems like I cut a little too deep. I'll update the tests too.
(Assignee)

Comment 7

3 years ago
Created attachment 8729464 [details] [diff] [review]
[PATCH WIP v3] Remove dependencies to the mozApps-related APIs from the audio channel code

Third attempt.
Attachment #8728990 - Attachment is obsolete: true
Attachment #8728990 - Flags: feedback?(fabrice)
(Assignee)

Comment 8

3 years ago
Created attachment 8730142 [details] [diff] [review]
[PATCH WIP v4] Remove dependencies to the mozApps-related APIs from the audio channel code

Refreshed patch, applies to the current pine branch.
Attachment #8729464 - Attachment is obsolete: true
No longer blocks: 1252143
(Assignee)

Comment 9

3 years ago
Created attachment 8738144 [details] [diff] [review]
[PATCH v5] Bug 1254282 - Remove dependencies to the mozApps-related APIs from the audio channel code

Refreshed patch with security policy adjusted: there's no restrictions anymore on which window can grab which channel. As discussed this really doesn't seem to be a big deal and will ensure we'll not lose functionality in the affected apps (clock, sms & phone, etc...). Let's land this so we can move forward with removing mozApps.
Attachment #8730142 - Attachment is obsolete: true
Attachment #8738144 - Flags: review?(fabrice)

Comment 10

3 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/116990543
(Reporter)

Comment 11

3 years ago
Comment on attachment 8738144 [details] [diff] [review]
[PATCH v5] Bug 1254282 - Remove dependencies to the mozApps-related APIs from the audio channel code

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

lgtm, thanks!

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +110,5 @@
>      return NS_OK;
>    }
>  
> +  if (nsContentUtils::IsChromeDoc(doc)) {
> +    return NS_OK; // System app

Any gaia app actually, not just the system one.
Attachment #8738144 - Flags: review?(fabrice) → review+
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/projects/pine/rev/58921658aef9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 13

3 years ago
Ben Francis changed story state to accepted in Pivotal Tracker
Status: RESOLVED → VERIFIED

Comment 14

3 years ago
Ben Francis changed story state to accepted in Pivotal Tracker
(Assignee)

Updated

3 years ago
Blocks: 1272270
You need to log in before you can comment on or make changes to this bug.