Closed Bug 1217303 Opened 4 years ago Closed 4 years ago

Remove mozContentEvent and mozChromeEvent about controlling System's audio channel in shell.js

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
FxOS-S10 (30Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: evanxd, Assigned: evanxd)

Details

Attachments

(1 file, 2 obsolete files)

Remove mozContentEvent and mozChromeEvent about controlling System's audio channel in shell.js. Please refer to https://hg.mozilla.org/mozilla-central/diff/ef1d868c5a85/b2g/chrome/content/shell.js.
Assignee: nobody → evan
Attached patch bug-1217303.patch (obsolete) — Splinter Review
Attached patch bug-1217303.patch (obsolete) — Splinter Review
Attachment #8677338 - Attachment is obsolete: true
Comment on attachment 8677351 [details] [diff] [review]
bug-1217303.patch

Hi baku,

Could you review the patch?

Because we already exported System app's allowed audio channels in `navigator.mozAudioChannelManager.allowedAudioChannels`, we don't need the mozChromeEvent and mozContentEvent to control the audio channels anymore. I remove the useless code in the patch.

Thanks.
Attachment #8677351 - Flags: review?(amarchesini)
Comment on attachment 8677351 [details] [diff] [review]
bug-1217303.patch

This patch is conflicted with current code base. Will update it.
Attachment #8677351 - Flags: review?(amarchesini)
Attachment #8677351 - Attachment is obsolete: true
Comment on attachment 8677931 [details] [diff] [review]
bug-1217303.patch

Hi baku,

Could you review the patch?

Thanks.
Attachment #8677931 - Flags: review?(amarchesini)
Thanks Alastor.
Comment on attachment 8677931 [details] [diff] [review]
bug-1217303.patch

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

::: b2g/chrome/content/shell.js
@@ +406,2 @@
>      let audioChannels = systemAppFrame.allowedAudioChannels;
>      audioChannels && audioChannels.forEach(function(audioChannel) {

do we still need this?
Attachment #8677931 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #9)
> Comment on attachment 8677931 [details] [diff] [review]
> bug-1217303.patch
> 
> Review of attachment 8677931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/shell.js
> @@ +406,2 @@
> >      let audioChannels = systemAppFrame.allowedAudioChannels;
> >      audioChannels && audioChannels.forEach(function(audioChannel) {
> 
> do we still need this?

Yes, we still need this to set System app's audio channels as unmuted by default. Because some audio in System app will be played, before AudioChannelService[1] in Gaia is loaded.
And thanks for the review.
Retrigger treeherder tasks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffcb9452d41d

Once all tasks are good, I'll do `checkin-needed`.
Treeherder tasks are all passed. Do checkin-needed!
Keywords: checkin-needed
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #10)
> Yes, we still need this to set System app's audio channels as unmuted by
> default. Because some audio in System app will be played, before
> AudioChannelService[1] in Gaia is loaded.

Can we initialize AudioChannelService before the first boot sound?
Flags: needinfo?(evan)
If so please file another bug and fix it.
https://hg.mozilla.org/mozilla-central/rev/279b06d77e81
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S10 (30Oct)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15)
> (In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #10)
> > Yes, we still need this to set System app's audio channels as unmuted by
> > default. Because some audio in System app will be played, before
> > AudioChannelService[1] in Gaia is loaded.
> 
> Can we initialize AudioChannelService before the first boot sound?

Yes, I think we can initialize AudioChannelService before the first boot sound. Already filed the Bug 1221022 to fix it.
Flags: needinfo?(evan)
You need to log in before you can comment on or make changes to this bug.