Closed Bug 1165134 Opened 10 years ago Closed 9 years ago

Add new MozChromeEvent/MozContentEvent to allow System app to control its own audio channels

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: evanxd, Assigned: evanxd)

References

Details

Attachments

(1 file, 4 obsolete files)

Add new MozChromeEvent/MozContentEvent in shell.js to allow System app to control its own audio channels.
Attached patch bug-1165134.patch (obsolete) — Splinter Review
Hi :baku, Could you help to review the patch for shell.js? Thanks.
Attachment #8606047 - Flags: review?(amarchesini)
:baku, one more question about `shell.sendChromeEvent` method in shell.js. We only can do `shell.sendChromeEvent` after the `MozAfterPaint` event is triggered, right? In my experiment, if I do `shell.sendChromeEvent` too early(before `MozAfterPaint` event is triggered), System app just cannot receive the MozChromeEvent. Is above correct? Or I misunderstood something? Thank.
Flags: needinfo?(amarchesini)
Depends on: 1157140, 1113086
Comment on attachment 8606047 [details] [diff] [review] bug-1165134.patch Review of attachment 8606047 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +843,5 @@ > }; > > +let SystemAppMozBrowserHelper = { > + handleEvent: function systemAppMozBrowser_handleEvent(detail) { > + let request; this is a nice C style approach, but can you initialize the variable when you need it? Otherwise I don't understand why just 'request' and 'name': they are not fully shared between all the cases of the switch. @@ +848,5 @@ > + let name; > + switch (detail.type) { > + case 'system-audiochannel-list': > + let audioChannels = []; > + shell.allowedAudioChannels.forEach(function(audioChannel) { shell.allowedAudioChannels.forEach(function(value, name) { audioChannels.push(name); }); @@ +857,5 @@ > + audioChannels: audioChannels > + }); > + break; > + case 'system-audiochannel-mute': > + name = detail.name; let name @@ +859,5 @@ > + break; > + case 'system-audiochannel-mute': > + name = detail.name; > + let isMuted = detail.isMuted; > + request = shell.allowedAudioChannels.get(name).setMuted(isMuted); let request @@ +876,5 @@ > + }); > + }; > + break; > + case 'system-audiochannel-volume': > + name = detail.name; let name @@ +878,5 @@ > + break; > + case 'system-audiochannel-volume': > + name = detail.name; > + let volume = detail.volume; > + request = shell.allowedAudioChannels.get(name).setVolume(volume); let request
Attachment #8606047 - Flags: review?(amarchesini) → review+
Depends on: 1166172
Depends on: 1167077
No longer depends on: 1113086
Depends on: 1167465
Attachment #8609104 - Attachment is obsolete: true
For some reasons, I just could not push try even I had registered level 1 in Bug 988801. Ricky will help this, and thanks.
(In reply to Andrea Marchesini (:baku) from comment #3) > Comment on attachment 8606047 [details] [diff] [review] > bug-1165134.patch > > Review of attachment 8606047 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/chrome/content/shell.js > @@ +843,5 @@ > > }; > > > > +let SystemAppMozBrowserHelper = { > > + handleEvent: function systemAppMozBrowser_handleEvent(detail) { > > + let request; > > this is a nice C style approach, but can you initialize the variable when > you need it? > Otherwise I don't understand why just 'request' and 'name': they are not > fully shared between all the cases of the switch. > > @@ +848,5 @@ > > + let name; > > + switch (detail.type) { > > + case 'system-audiochannel-list': > > + let audioChannels = []; > > + shell.allowedAudioChannels.forEach(function(audioChannel) { > > shell.allowedAudioChannels.forEach(function(value, name) { > audioChannels.push(name); > }); > > @@ +857,5 @@ > > + audioChannels: audioChannels > > + }); > > + break; > > + case 'system-audiochannel-mute': > > + name = detail.name; > > let name > > @@ +859,5 @@ > > + break; > > + case 'system-audiochannel-mute': > > + name = detail.name; > > + let isMuted = detail.isMuted; > > + request = shell.allowedAudioChannels.get(name).setMuted(isMuted); > > let request > > @@ +876,5 @@ > > + }); > > + }; > > + break; > > + case 'system-audiochannel-volume': > > + name = detail.name; > > let name We cannot declare two same variables, see it in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let. > > @@ +878,5 @@ > > + break; > > + case 'system-audiochannel-volume': > > + name = detail.name; > > + let volume = detail.volume; > > + request = shell.allowedAudioChannels.get(name).setVolume(volume); > > let request
Fabrice, you know more about this. Can you read comment 2?
Flags: needinfo?(amarchesini) → needinfo?(fabrice)
(In reply to (Vacation 5/22 - 6/1) Evan Tseng [:evanxd][:愛聞插低] from comment #2) > :baku, one more question about `shell.sendChromeEvent` method in shell.js. > We only can do `shell.sendChromeEvent` after the `MozAfterPaint` event is > triggered, right? > > In my experiment, if I do `shell.sendChromeEvent` too early(before > `MozAfterPaint` event is triggered), System app just cannot receive the > MozChromeEvent. > > Is above correct? Or I misunderstood something? > > Thank. We should buffer events if you send them from SystemAppProxy.jsm Also, don't send mozChromeEvents with a `type`, that leads to performance issue. Rather, create a custom element with your own name, like for instance http://mxr.mozilla.org/mozilla-central/source/b2g/components/InterAppCommUIGlue.js#66
Flags: needinfo?(fabrice)
Really thanks for Ricky's help.
For Comment 2, I created Bug 1167465.
Evan, please fix the event naming as I asked in comment 9.
Hi Fabrice, Sorry, didn't see that before. Sure, let me fix it.
Hi Fabrice, Juse one thing to make sure. if we do ``` // type => action shell.sendChromeEvent({ action: 'system-audiochannel-list', audioChannels: audioChannels }); ``` , there is no performance issue and it is good, right? And currently, many statements([1], [2], [3], and more) about `sendChromeEvent` are just with `type` in shell.js, they also lead to performance issue, right? Thanks. [1]: https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#165-169 [2]: https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#467-470 [3]: https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#490-493
Flags: needinfo?(fabrice)
And one more thing. Fabrice, do you know why the naming(type) cause the performance issue?
Evan, No, switching from `type` to `action` is not good, and yes we have chromeEvent uses that I'd like to see disappear. You need to use your own event name, like in http://mxr.mozilla.org/mozilla-central/source/b2g/components/InterAppCommUIGlue.js#66 The reason is that if all we send is a `mozChromeEvent` event, we end up with lots of event listeners for this event in the system app, and we need to eventually run a large number of handler that just check if they can process the event's `type`. Unfortunately that can take time... we used to spend 30ms just to reach the right event handler to launch apps for instance!
Flags: needinfo?(fabrice)
Attachment #8617247 - Attachment is obsolete: true
Hi Fabrice, Got you. I just updated the patch for your comments. See it in[1]. Thanks. [1]: https://bug1165134.bugzilla.mozilla.org/attachment.cgi?id=8617247
Wait for Bug 1174121. Once it is fixed, then we can push try again.
The try for Comment 21: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cebadab1d8 Looks like there are some problems there, all Gij tasks were not passed. Start to debug.
Try for changing gaia code in bug-1166172 branch[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a45c42422882 If the Gij tasks are passed, I think we just need to apply the gaia change[1]. [1]: https://github.com/evanxd/gaia/tree/bug-1166172
If we change the timing of sending the content event[1][2], the try server tasks are good. So we can say Bug 1166172 just block this bug currently. Let's fix that. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1166172 [2]: https://github.com/mozilla-b2g/gaia/pull/30120/files
Attachment #8606047 - Attachment is obsolete: true
Comment on attachment 8621523 [details] [diff] [review] bug-1165134.patch - Updated the patch for review comments This patch is a duplicate of the r+ patch[1] with fixing some nits[2]. And the try push for it is also good[3]. Let's do `checkin-needed` and land the code. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1165134#c3 [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1165134#c9 [3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c6673ead1f
Attachment #8621523 - Flags: review+
Keywords: checkin-needed
Attachment #8621523 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: