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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: evanxd, Assigned: evanxd)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Add new MozChromeEvent/MozContentEvent in shell.js to allow System app to control its own audio channels.
(Assignee)

Comment 1

4 years ago
Created attachment 8606047 [details] [diff] [review]
bug-1165134.patch

Hi :baku,

Could you help to review the patch for shell.js?

Thanks.
Attachment #8606047 - Flags: review?(amarchesini)
(Assignee)

Comment 2

4 years ago
: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)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Updated

4 years ago
Depends on: 1166172
(Assignee)

Updated

4 years ago
Depends on: 1167077
No longer depends on: 1113086
(Assignee)

Comment 4

4 years ago
Created attachment 8609104 [details] [diff] [review]
bug-1165134.patch  - Updated the patch for review comments
(Assignee)

Updated

4 years ago
Depends on: 1167465

Updated

4 years ago
Blocks: 1113086
(Assignee)

Comment 5

3 years ago
Created attachment 8617247 [details] [diff] [review]
bug-1165134.patch - Updated the patch for review comments
Attachment #8609104 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
For some reasons, I just could not push try even I had registered level 1 in Bug 988801. Ricky will help this, and thanks.
(Assignee)

Comment 7

3 years ago
(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)
(Assignee)

Comment 11

3 years ago
Really thanks for Ricky's help.
(Assignee)

Comment 12

3 years ago
For Comment 2, I created Bug 1167465.
(Assignee)

Comment 14

3 years ago
Sorry, this is just the correct one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=336280f45d38
Evan, please fix the event naming as I asked in comment 9.
(Assignee)

Comment 16

3 years ago
Hi Fabrice,

Sorry, didn't see that before.
Sure, let me fix it.
(Assignee)

Comment 17

3 years ago
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)
(Assignee)

Comment 18

3 years ago
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)
(Assignee)

Comment 20

3 years ago
Created attachment 8621523 [details] [diff] [review]
bug-1165134.patch - Updated the patch for review comments
Attachment #8617247 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
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
(Assignee)

Comment 22

3 years ago
Wait for Bug 1174121. Once it is fixed, then we can push try again.
(Assignee)

Comment 23

3 years ago
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.
(Assignee)

Comment 24

3 years ago
Created attachment 8622200 [details] [diff] [review]
Debug for the try service error

Debug for the try service error[1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cebadab1d8
(Assignee)

Comment 26

3 years ago
A new try push for all gecko and gaia tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c6673ead1f
(Assignee)

Comment 27

3 years ago
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
(Assignee)

Comment 28

3 years ago
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
(Assignee)

Comment 29

3 years ago
The try server tasks in Comment 28 are good[1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a45c42422882.
(Assignee)

Updated

3 years ago
Attachment #8606047 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Attachment #8621523 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ef1d868c5a85
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.