767311 - Migrate volume change to mozSettings

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

The scenario of volume change now is:
* Listen to keyup/keydown event in shell.js
* call changeVolume(+1/-1)

This is duplicated in gaia, which also changes volume by listening to keyup/keydown of volume buttons.

The patch tried to move volume change into mozSettings, named 'sound.volume'.
And set volume directly according to SettingsListener.observe

It will need gaia's support. I will comment with gaia's pull request later.

Reference gaia issue:
https://github.com/mozilla-b2g/gaia/issues/1668

Comment 1

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

Attachment: patch for change volume's new scenario

Comment 2

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

Related gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1877

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 635676 [details] [diff] [review]
patch for change volume's new scenario

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

Thanks for taking this! Just move a little bit the code and it would be fine.

::: b2g/chrome/content/shell.js
@@ +153,5 @@
> +    
> +    let self = this;
> +    SettingsListener.observe("sound.volume", 5, function(value) {
> +      self.setVolume(value);
> +    });

Move this code and setVolume to settings.js. You can inline setVolume inside this observer.

@@ +190,5 @@
>      let currentVolume = audioManager.masterVolume;
>      let newStep = Math.round(steps * Math.sqrt(currentVolume)) + delta;
>      let volume = (newStep / steps) * (newStep / steps);
>  
> +    this.setVolume(volume);

I would like to remove all this logic to have it in Gaia. Remove it completely.

@@ +202,5 @@
> +    
> +    let audioManager = Services.audioManager;
> +    if (!audioManager)
> +      return;
> +    

Check audioManager on top of the method.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Vivien Nicolas (:vingtetun) from comment #3)
> Comment on attachment 635676 [details] [diff] [review]
> patch for change volume's new scenario
> 
> Review of attachment 635676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking this! Just move a little bit the code and it would be fine.
> 
> ::: b2g/chrome/content/shell.js
> @@ +153,5 @@
> > +    
> > +    let self = this;
> > +    SettingsListener.observe("sound.volume", 5, function(value) {
> > +      self.setVolume(value);
> > +    });
> 
> Move this code and setVolume to settings.js. You can inline setVolume inside
> this observer.
> 

Yeah that make sense.

> @@ +190,5 @@
> >      let currentVolume = audioManager.masterVolume;
> >      let newStep = Math.round(steps * Math.sqrt(currentVolume)) + delta;
> >      let volume = (newStep / steps) * (newStep / steps);
> >  
> > +    this.setVolume(volume);
> 
> I would like to remove all this logic to have it in Gaia. Remove it
> completely.

So audioManager.masterVolume = "sound.volume" value with out any calculation?

> 
> @@ +202,5 @@
> > +    
> > +    let audioManager = Services.audioManager;
> > +    if (!audioManager)
> > +      return;
> > +    
> 
> Check audioManager on top of the method.

and move that to settings.js?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #4)
> > 
> > I would like to remove all this logic to have it in Gaia. Remove it
> > completely.
> 
> So audioManager.masterVolume = "sound.volume" value with out any calculation?


Yep. I expect the calculation to be done in Gaia, not here.
 
> > 
> > @@ +202,5 @@
> > > +    
> > > +    let audioManager = Services.audioManager;
> > > +    if (!audioManager)
> > > +      return;
> > > +    
> > 
> > Check audioManager on top of the method.
> 
> and move that to settings.js?

Correct.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

BTW, @alive, I propose we change the settings name to "audio.volume.master", how do you think?

Comment 7

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

I think if we are going to control volume on different part like system-volume, notification-volume, ring-volume, and media-volume, the parameter should also be separated. Right? I couldn't image how we can separate these in gaia level.
So "audio.volume.master" means os-wide volume? Make sense.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alive Kuo from comment #7)
> So "audio.volume.master" means os-wide volume?

Yes.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo from comment #7)
> I think if we are going to control volume on different part like
> system-volume, notification-volume, ring-volume, and media-volume, the
> parameter should also be separated. Right? I couldn't image how we can
> separate these in gaia level.

There is no way for Gaia to control that for now.

Comment 10

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

Attachment: patch for volume setting, rev#2

Patch modified according to the comments

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alive Kuo from comment #10)
> Created attachment 636222 [details] [diff] [review]
> patch for volume setting, rev#2
> 
> Patch modified according to the comments

Did you test you patched Gecko? I think you should also move the topmost part here:

http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#31

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 636222 [details] [diff] [review]
patch for volume setting, rev#2

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

::: b2g/chrome/content/settings.js
@@ +54,5 @@
> +  {
> +    dump('======get audioManager failed');
> +    return;
> +  }
> +  

if (!audioManager)
  return;

@@ +61,5 @@
> +  if (value < 0)
> +    value = 0;
> +
> +  audioManager.masterVolume = value;
> +  dump('=======',value);

audioManager.masterVolume = Math.min(0, Math.max(value, 1));

Comment 13

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #11)
> Did you test you patched Gecko? I think you should also move the topmost
> part here:
> 
> http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#31

Yes I tested and worked fine.
Did you mean move the single 'lazyservicegetter' line or all the '#ifdef'~'#endif' part of audioManager?

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alive Kuo from comment #13)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #11)
> > Did you test you patched Gecko? I think you should also move the topmost
> > part here:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#31
> 
> Yes I tested and worked fine.
> Did you mean move the single 'lazyservicegetter' line or all the
> '#ifdef'~'#endif' part of audioManager?

Yes that's what I meant. Vivien, is that correct?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #14)
> (In reply to Alive Kuo from comment #13)
> > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #11)
> > > Did you test you patched Gecko? I think you should also move the topmost
> > > part here:
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#31
> > 
> > Yes I tested and worked fine.
> > Did you mean move the single 'lazyservicegetter' line or all the
> > '#ifdef'~'#endif' part of audioManager?
> 
> Yes that's what I meant. Vivien, is that correct?

What Tim says.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Progress?

Comment 17

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

Attachment: patch rev#3

Comment 18

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

(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #14)
> > (In reply to Alive Kuo from comment #13)
> > > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #11)
> > > > Did you test you patched Gecko? I think you should also move the topmost
> > > > part here:
> > > > 
> > > > http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#31
> > > 
> > > Yes I tested and worked fine.
> > > Did you mean move the single 'lazyservicegetter' line or all the
> > > '#ifdef'~'#endif' part of audioManager?
> > 
> > Yes that's what I meant. Vivien, is that correct?
> 
> What Tim says.

Move those line(s) to settings.js will result in booting fail. Dunno what happend :|
Need further investigation.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) from comment #15)
> > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #14)
> > > (In reply to Alive Kuo from comment #13)
> > > > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #11)
> > > > > Did you test you patched Gecko? I think you should also move the topmost
> > > > > part here:
> > > > > 
> > > > > http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#31
> > > > 
> > > > Yes I tested and worked fine.
> > > > Did you mean move the single 'lazyservicegetter' line or all the
> > > > '#ifdef'~'#endif' part of audioManager?
> > > 
> > > Yes that's what I meant. Vivien, is that correct?
> > 
> > What Tim says.
> 
> Move those line(s) to settings.js will result in booting fail. Dunno what
> happend :|
> Need further investigation.

What is breaking? Is it because of the ordering of the files in shell.xul? Or because of the definition of the audioManager?

Comment 20

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

@vingtetun, the actual reason is redefine of `const Cu = Components.utils;`
Because it is already defined in head of shell.js

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo from comment #20)
> @vingtetun, the actual reason is redefine of `const Cu = Components.utils;`
> Because it is already defined in head of shell.js

Can you fix that?

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Vivien Nicolas (:vingtetun) from comment #21)
> (In reply to Alive Kuo from comment #20)
> > @vingtetun, the actual reason is redefine of `const Cu = Components.utils;`
> > Because it is already defined in head of shell.js
> 
> Can you fix that?

There are many, many constants declared in shell.js that is used in settings.js. Do you want to decouple ALL THAT? I would say that should go to another bug and let's follow the *consistency* here.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #22)
> (In reply to Vivien Nicolas (:vingtetun) from comment #21)
> > (In reply to Alive Kuo from comment #20)
> > > @vingtetun, the actual reason is redefine of `const Cu = Components.utils;`
> > > Because it is already defined in head of shell.js
> > 
> > Can you fix that?
> 
> There are many, many constants declared in shell.js that is used in
> settings.js. Do you want to decouple ALL THAT? I would say that should go to
> another bug and let's follow the *consistency* here.

I don't understand why you said that. I'm not going to review a patch where the author says it does *not* work. It seems like there is some incomprehension here...

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Vivien Nicolas (:vingtetun) from comment #23)
> I don't understand why you said that. I'm not going to review a patch where
> the author says it does *not* work. It seems like there is some
> incomprehension here...

Hum, there are some miscommunication here.

Patch v3 will work, ironically, moving topmost part to settings.js as I suggested in comment 11 will not work. The reason comment 11 will not work is because Cu, Cc, etc can not be defined in both files.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What is need to be done to have this bug moving forward?

Comment 26

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

Attachment: checkin patch

the patch to be checkin

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8e385922fc

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f8e385922fc