971554 - [System2] Instantiable SoundManager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

We need sound_manager.js to be instantiable and let bootstrap or its parent module to instantiate it.
Also jsdoc + unit test improvement wanted.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

Alive,

I found the mute behavior is inconsistent at [1]. It only mutes notification while muting but unmutes both notification and content while unmuting.

Should we mute both of them and unmuting both of them?

[1] https://github.com/mozilla-b2g/gaia/blob/f1f99c6f880d2adbf5886a8dbe6345141c2d5626/apps/system/js/sound_manager.js#L29-L50

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

A WIP patch can be found here:
https://github.com/huchengtw-moz/gaia/commit/4e2b4eb3a363f2ee644a0d963032153cddb25de9

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Please do not change the behavior, it's by design.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: patch for bubble tea

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: PR for master

These two PR are exactly the same but for different branch.

Change items:
1. rewrap the SoundManager as a class, no behavior changed.
2. add about 20 unit tests for SoundManager
3. update bootstrap.js and bootstrap_test.js
4. add MockSoundManager
5. fix jshint of SoundManager

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8381170 [details] [review]
PR for master

clear review flag for checking the jsdoc parts.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8381167 [details] [review]
patch for bubble tea

Hi alive,

I had changed the patch to enable the jsdoc generation of SoundManager. Please review this version. Thanks.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8381170 [details] [review]
PR for master

Since our patches go to bubble tea, we don't need this one.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8381167 [details] [review]
patch for bubble tea

Nice big work. r+ with nits. Thanks!

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to bubble-tea:
https://github.com/mozilla-b2g/gaia/commit/99fcb153535a3e846f0df2a8b0752d85cc1f975e

Comment 11

[[e-mail]]

Attachment: mozilla-b2g:master PR#17494

Comment 12

[[e-mail]]

travis all green, merged https://github.com/mozilla-b2g/gaia/commit/c377f41d017d4b099e92fd7ecf67d9cd1cee1292

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks Fred,

This is a hard bug....

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/34cdf08e963c1fde841828b69ae02fab43828573 for breaking gaia-unit tests on TBPL:

https://tbpl.mozilla.org/php/getParsedLog.php?id=36588967&tree=B2g-Inbound

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Reopen it because of comment 14.

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8381170 [details] [review]
PR for master

Rebase to master.

travis all green: https://travis-ci.org/mozilla-b2g/gaia/builds/21645503

Comment 17

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/9df14421065bcc5fcb94735f6ab7cb34d6ddc643

thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Sorry, I accidentally committed some bad changesets to the Gaia repo which you had the misfortunate of pushing on top of. Rather than trying to rebase it all around, I ended up just reverting to the last good changeset, which means your push got lost. I'm *VERY* sorry for the inconvenience I've caused you :(

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

reland to master:
https://github.com/mozilla-b2g/gaia/commit/eee964fc7e8dc519218dee6b72595ad0f7dd8231

Comment 20

[Carsten Book [:Tomcat]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #19)
> reland to master:
> https://github.com/mozilla-b2g/gaia/commit/
> eee964fc7e8dc519218dee6b72595ad0f7dd8231

i think this have broken marionette tests again

Comment 21

[Carsten Book [:Tomcat]]

yeah red mnw tests results started with this push - https://tbpl.mozilla.org/php/getParsedLog.php?id=36982015&tree=B2g-Inbound

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

the commit eee964fc7e8dc519218dee6b72595ad0f7dd8231 is reverted.

Comment 23

[John Hu [:johnhu][:johu][:醬糊小弟]]

PR: https://github.com/mozilla-b2g/gaia/pull/17807

reland to master: https://github.com/mozilla-b2g/gaia/commit/3c23b5013e62d6f059d9a0b9f9e8c8db0c3aa2fe


travis is all green: https://travis-ci.org/mozilla-b2g/gaia/builds/21990686

I had tried try-server. But it gives me timed-out all the time:
Bug 982995 - Intermittent TEST-UNEXPECTED-FAIL | test_sms_add_contact.py test_sms_add_contact.TestSmsAddContact.test_sms_add_contact | (after TimeoutException: TimeoutException: Timed out after 10.0 seconds)
TEST-UNEXPECTED-FAIL | test_context_menu_activity_picker.py test_context_menu_activity_picker.TestContextMenuActivityPicker.test_context_menu_activity_picker |

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Reverted for causing bug 991385.
https://github.com/mozilla-b2g/gaia/commit/d9a574284d672f532f7c562a091bb01f531202b1

Comment 25

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: patch for master

Request to review the patch because a regression of previous patch is fixed.

The change between this one and https://github.com/mozilla-b2g/gaia/pull/17807 is at line 349[1] which calls this.headsetVolumeup() only when user volumes up and the headset is plugged in.

[1] https://github.com/mozilla-b2g/gaia/pull/18007/files#diff-6cb5950c74c73ee2f0c39a8b418d1548R349

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8402483 [details] [review]
patch for master

r+ if you have unit tests for the failure.

Comment 27

[:gerard-majax]

This is r+, Travis green, and it may be blocking/conflicting with bug 1006356, what are we waiting to land this?

Comment 28

[Gregor Wagner [:gwagner]]

(In reply to Alexandre LISSY :gerard-majax from comment #27)
> This is r+, Travis green, and it may be blocking/conflicting with bug
> 1006356, what are we waiting to land this?

1006356 is a cert blocker for 1.3 so we shouldn't delay it with merging this in the next 24 hours. Lets coordinate if you want to land this in the next few days.

Comment 29

[John Hu [:johnhu][:johu][:醬糊小弟]]

We got a call to post-pone the landing of System2. We will try to resolve it after bug 1006356 landed.

Comment 30

[:gerard-majax]

I'm concerned this has not yet landed ; it's blocking writing new tests, as of bug 1011000.

Comment 31

[John Hu [:johnhu][:johu][:醬糊小弟]]

Alexandre,

The PR had rebased. I had discussed this one with Tim. We will not land this patch until the gaia-try green, and I will dogfeed this patch at least a week before landing it.

It is welcome that you may help to test this patch on your site.

Comment 32

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:

https://github.com/mozilla-b2g/gaia/commit/9d822c4666c507da8ffe9967df530aa6980acbf8

Comment 33

[ankit93040]

Hi Alive

Why don't you uplift this to V2.0 as well?

thanks!

Comment 34

[(inactive after 6/18) Alive Kuo [:alive]]

This is not a 2.0 feature but pure engineering work item.

Comment 35

[ankit93040]

Attachment: sound_manager_test.patch

Hi Alive,John

I need little help from you. I am not good at unit test cases.
I was trying to write Unit test case for v2.0 for sound_manager.
I almost copied John's Unit test case file for sound_manager.
Here is the problem I am facing - 
1.> Even after volumeup/down/mute/unmute the volume of the channel is not changing.
Will you please help!

thanks!
Ankit

Comment 36

[ankit93040]

In addition to comment#35

If I have initialized the volume for a channel to 5 then even after mute/unmute it reamins 5.
If I have initialized the volume for a channel to 1 then even after volume up/down it reamins same.

Comment 37

[John Hu [:johnhu][:johu][:醬糊小弟]]

Ankit93040,

Would you mind to share your code with us? Without the code, we cannot find the real problem.

Comment 38

[ankit93040]

Hi John

I want to know one thing when we say - window.dispatchEvent(new CustomEvent('volumeup'));

Have we defined this volumeup event somewhere else in some other file? (i mean in mock/test file)

Actually what ever Unit test I uploaded :- 
https://bug971554.bugzilla.mozilla.org/attachment.cgi?id=8464486
when i tested it against mozilla v2.0 sound_manager.js file there are many errors.

One of the errors it shows when I wrote this - window.dispatchEvent(new CustomEvent('mute'));
The error was "currentVolume is undefined".
I feel that mute event looks into currentVolume.

So, I wanted to know have you defined these volumeup events in some file?

thanks!

Comment 39

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks. I will test it when I am free. Keep ni in myself to remind me.

Comment 40

[John Hu [:johnhu][:johu][:醬糊小弟]]

Ankit,

I had checked your test case. The main difference between us is that:
* I create my local-owned sound manager and testing with it. And you are trying to use the window.soundManager. It depends on the l10n.once or l10n.ready. The main reason your version won't work is that l10n is loaded after the "sound_manager".

The suggestion to fix it is that you may move the require('sound_manager.js') into the first suiteSetup(). After that you may need to change the suiteSetup() to suiteSetup(done) and require('.../sound_manager.js', done);