870608 - [Volume][Settings]After flashing the volume should not be set at max

Status: RESOLVED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

## Environment :
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/cfef36c0c8bc
Gaia   a80be95f553de517e5d8a159e04511cda5e38be4
BuildID 20130409070205
Version 18.0
Unagi
  
## Repro :
1. flash the device
2. go to settings -> sounds
3. look at the volume

## Expected :
it's set midway

## Actual :
everything is set to max

## Note :
- Every time I forget that I just flashed it, I end up having my ears blown.
- happens in all versions.  Rather annoying.
- I believe this is the case for resetting the phone as well.

Comment 1

[Daniel Coloma:dcoloma]

Don't think we should block on this one

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Given that users will not be regularly flashing phones, this is not a blocker but a nice-to-have for testers.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

This is not just on flashing, also normal restart (master, on Gaia 6dbaa14222e624f0fc436c2f5b4730409b8491f8, B2G d07cc703cfa8205fd77b2d8258fc0069f2d718a0 against mozilla-central 4ac6c72b06c8+) triggers the volume to reset to max. Which is highly annoying.

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Re-nominating for tef to verify if a restart also triggers a volume reset on 1.0.1.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

When we currently start up the phone we add a bunch of observers for the settings related to the volume. All good. However, we skip the check if we have any pending requests. When the first result comes in, it can trigger a chain of reactions which then spawns new pending requests, causing the other loaded settings to be omitted and the volume of f.e. the ringer to be kept at the original of 15. This patch makes sure that the initial load event for every setting will bubble through.

Comment 6

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

Comment on attachment 755236 [details] [diff] [review]
Patch

Redirecting review as Alive is the original care-taker of the sound manager code :)

Comment 7

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

The problem of comment 0 is because we set the default volume to max(from long time ago).
We could just set it to arbitrary value we like.

I tried on master, and after rebooting the notification/ringer volume is not max.
Will try on v1.0.1 soon.

Comment 8

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

Comment on attachment 755236 [details] [diff] [review]
Patch

Sorry, I don't think this fixes anything.

The pending request only happens when the user uses the volume rockers to adjust the volume level. For your guess, it's nearly impossible because the user won't press the volume rocker 'just before the observer callback and after booting'. It's a very short interval.

BTW, the pendingRequestCount is a number, not a object. I think pendingRequestCount[channel] will cause trouble.

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 755236 [details] [diff] [review]
Patch

@Alive, no this is not the case. The settingsObserver also fires directly after startup of the phone and sets the initial values. You can see this behavior in https://github.com/mozilla-b2g/gaia/blob/master/shared/js/settings_listener.js#L37. This means that for all of these keys the initial value will be read immediately after startup but it'll be ignored for everyone who doesn't finish first, thus not setting the volume.

The change of pendingRequestCount was unintentional, I've got that out of the way and resquashed.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

It's also easily verifiable on Gaia in Firefox Nightly if you don't want to flash phone.

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

FYI, add on line 283: `console.log('SettingsListener.observe', channel, volume, pendingRequestCount);`

[13:51:14.604] "SettingsListener.observe" "alarm" 15 0
[13:51:14.605] "SettingsListener.observe" "notification" 11 2
[13:51:14.605] "SettingsListener.observe" "telephony" 5 2
[13:51:14.606] "SettingsListener.observe" "content" 15 2
[13:51:14.606] "SettingsListener.observe" "bt_sco" 15 2

And you'll see that at the moment stuff is not put through because of this.

Comment 12

[Daniel Coloma:dcoloma]

still thinking it is not tef+

Comment 13

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

Comment on attachment 755236 [details] [diff] [review]
Patch

OK I know what happens.
If the pendingRequestCount is not 0,
we should find out what's happening instead skip it.

The problem is the fetchCachedVolume here, due to some race condition, it may return the result too late.

So the solution is delay the call to fetchCachedVolume function call after all of the channels are retrieved first time.

Yes this is my design fail, but we should provide a real fix.

If you don't want to work on this I will take it, thanks.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 755236 [details] [diff] [review]
Patch

Alive, thanks, I've changed the PR to take that into accont. Good catch. I've also seen that `CACHE_CETIMES` is declared but we write `CACHE_CETIME` to asyncStorage, so to the undefined key. I've added it to the PR, if you want it out please say so.

Comment 15

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

Comment on attachment 755236 [details] [diff] [review]
Patch

Awesome. r=alive, thanks for your patch and patience.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Landed in https://github.com/mozilla-b2g/gaia/commit/979126f56c6ad3608770049f5ba71cf3e0c3857f

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

triage: agree with comment 2, tef-. should not block release with this.

Comment 19

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

tef? because dup 878740 is tef+

Comment 20

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

v101
https://github.com/mozilla-b2g/gaia/commit/bf10abc41a01516995a99f3c6727a612bbdfe755

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Alive Kuo [:alive] from comment #20)
> v101
> https://github.com/mozilla-b2g/gaia/commit/
> bf10abc41a01516995a99f3c6727a612bbdfe755

is this applicable to v1-train?  If so, can you please land on that branch?

Comment 22

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

(In reply to John Ford [:jhford] -- If you expect a reply from me, use needsinfo? instead of CC from comment #21)
> (In reply to Alive Kuo [:alive] from comment #20)
> > v101
> > https://github.com/mozilla-b2g/gaia/commit/
> > bf10abc41a01516995a99f3c6727a612bbdfe755
> 
> is this applicable to v1-train?  If so, can you please land on that branch?

No problem.

Comment 23

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

https://github.com/mozilla-b2g/gaia/commit/390104667c95a8bbd416fd95117577cbaa746901
v1-train