Bug 805333 adds an audio permission which allows content to compete for audio channels. More important audio channels are only available to more trusted apps types. Each channel has an associated permission, and the app type (web, privileged, certified) needs to be checked at the time of granting the permission to ensure that the type requirement is satisfied. The correspondence between app types and audio permission is: audio-normal: web, privileged, certified audio-content: web, privileged, certified audio-notification: privileged, certified audio-alarm: privileged, certified audio-telephony: certified audio-ringer: certified audio-publicnotification: certified I think there most likely just needs to be a check of this app type as part of the "expandPermissions" function in PermissionsInstaller.jsm

Hi Mounir, This is just a WIP patch but almost done. Hoping to have your feedback first for how to deploy the channel-to-permission mapping table. My design is shown as below: audio: { app: [ ALLOW_ACTION, // "normal" ALLOW_ACTION, // "content" DENY_ACTION, // "notification" DENY_ACTION, // "alarm" DENY_ACTION, // "telephony" DENY_ACTION, // "ringer" DENY_ACTION // "publicnotification" ], privileged: [ ALLOW_ACTION, // "normal" ALLOW_ACTION, // "content" ALLOW_ACTION, // "notification" ALLOW_ACTION, // "alarm" DENY_ACTION, // "telephony" DENY_ACTION, // "ringer" DENY_ACTION // "publicnotification" ], certified: [ ALLOW_ACTION, // "normal" ALLOW_ACTION, // "content" ALLOW_ACTION, // "notification" ALLOW_ACTION, // "alarm" ALLOW_ACTION, // "telephony" ALLOW_ACTION, // "ringer" ALLOW_ACTION // "publicnotification" ], channels: [ "normal", "content", "notification", "alarm", "telephony", "ringer", "publicnotification" ] }, Does that make sense to you?

Comment on attachment 689646 [details] [diff] [review] WIP Patch Review of attachment 689646 [details] [diff] [review]: ----------------------------------------------------------------- Can't we simply name the permissions: "audio-normal", "audio-content", "audio-notification", "audio-alarm", ... ? I feel like it would make that change *way* simpler and I would believe we do not have that much apps that would require to be fixed. The fix would actually be quite simple: just push a change to Gaia adding those permissions to the app that require them, then push this patch to m-b. Anyway, whatever you decide to do, Gene, I will be 100% offline for the next 8 days so you should unfortunately find someone else for the review.

(In reply to Mounir Lamouri (:mounir) (on vacation 100% offline until the 16th of December) from comment #2) > Comment on attachment 689646 [details] [diff] [review] > WIP Patch > > Review of attachment 689646 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can't we simply name the permissions: "audio-normal", "audio-content", > "audio-notification", "audio-alarm", ... ? I was following the design at Bug 805333, where they hoped to use an extra |channels| property to specify the multiple types for a single "audio" permission. That's why I have to deploy the app types in this way. I think it's still advantageous to refactor the "audio" part, which looks a bit cleaner from the point of view of the app's manifest. > Anyway, whatever you decide to do, Gene, I will be 100% offline for the next > 8 days so you should unfortunately find someone else for the review. No worries! Have a good vacation! :)

Comment on attachment 689646 [details] [diff] [review] WIP Patch Hi Jonas, May I have your feedback on this patch please? This is almost done but before asking for a formal review, I'd like to have your confirmation to see if comment #1 is a proper way to specify the channel-to-permission mapping given the app type.

Hi Jonas & Fabrice, 1. Please see comment #1 for how I deploy the channel-to-permission mapping table (by the given app type). 2. Due the consideration of *channels*, another big change is to make |.expandPermissions()| return an object with the following format: { 'permNames': (string array), 'permValues': (number array) } where each value in the 'permValues' array specifies the permission value (allow, deny... etc) for each permission name (audio-channel-normal, audio-channel-telephony... etc) in the 'permNames' array. Why we need to do this is because different expanded permissions could now have different permission values even if they are under the same app type (privileged, certified... etc). 3. All the callers of |.expandPermissions()| need to have the corresponding changes, including: a. SystemMessagePermissionsChecker.jsm b. PermissionPromptHelper.jsm c. test_bug808734.js 4. A minor code clean-up: s/PermissionsTable.jsm/PermissionsInstaller.jsm. Please feel free to let me know if I need to do this in another patch.

Actually, I think Mounir has a good point. I think we ended up creating something overly complicated here rather than just sticking to the pattern that we had. So maybe we should simply add "audio-channel-normal", "audio-channel-content", etc to the table, just like all the other types that we have. That mean we can revert expandPermissions to the simpler code that we had before. And it means that future gaia UI around implicit permissions doesn't need to have special code for audio channels. It would mean that we have to change the various gaia manifests to use the new syntax. I.e. the alarm app would have to change from permissions: { audio: { description: "...", channels: ["normal", "alarm"] }, ... } to permissions: { audio-channel-normal: { description: "...", }, audio-channel-alarm: { description: "...", }, ... } Gene: would you mind making this change. I'm sorry about the churn. The old syntax seemed like a good idea, but the amount of complexity just doesn't seem worth it. And the new syntax has the advantage that developers are forced to motivate each channel with a description.

Sounds great! Thanks for the good point! :) I'll come back with the refined patch soon.

Comment on attachment 690760 [details] Part 1, Gaia Change for Audio Permissions in Manifests r=me, be careful to land gaia side as fast as possible when gecko lands.

Life is simpler now. ;) Thanks for your review!

Comment on attachment 690762 [details] [diff] [review] Part 2, Gecko Change for Audio Permissions Models, V3 Review of attachment 690762 [details] [diff] [review]: ----------------------------------------------------------------- Yay! Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/971c2bbc41aa https://hg.mozilla.org/releases/mozilla-aurora/rev/267652713bbb https://hg.mozilla.org/releases/mozilla-beta/rev/337aa6725d31

Gaia merge: https://github.com/mozilla-b2g/gaia/commit/e580fb74e3430520168d388ef3759dd3d6ada870