904531 - [AudioManager] During boot sequence AudioManager should initialize the audio channel volume based on settings.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

Currently audio manager will initialize the master volume, max channel volume and routing path now. But it still needs to read current volume from settings then apply to audio backend.

Comment 1

[Marco Chen [:mchen]]

Attachment: bug-904531-AudioManager-init-vol

Issue:
  AudioManager didn't sync initial volume index for each channel types from settings DB, so user will find volume is too small until the first time of adjusting volume.

Solution:
  During AudioManager's constructor, it actively get volume indexes from settings DB for initializing the stream volumes.

Comment 2

[Michael Wu [:mwu]]

It's helpful to set dependencies on the bugs so people can find these follow ups.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 792130 [details] [diff] [review]
bug-904531-AudioManager-init-vol

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

Looks good overall. Thanks

::: dom/system/gonk/AudioManager.cpp
@@ +118,5 @@
> +  NS_IMETHOD Handle(const nsAString& aName, const JS::Value& aResult)
> +  {
> +    nsCOMPtr<nsIAudioManager> audioManager =
> +      do_GetService(NS_AUDIOMANAGER_CONTRACTID);
> +    if (JSVAL_IS_INT(aResult)) {

if (!JSVAL_IS_INT(aResult))
  return NS_OK;

or use NS_ENSURE_TRUE which would give us a warning. It seems like something would be very broken if we actually manage to hit that, so a warning would be nice.

@@ +142,5 @@
> +
> +  NS_IMETHOD HandleError(const nsAString& aName)
> +  {
> +    LOG("AudioChannelVolInitCallback::HandleError: %s\n",
> +      NS_LossyConvertUTF16toASCII(aName).get());

Why not NS_ConvertUTF16toUTF8? (not that it would matter much here)

@@ +384,5 @@
> +  nsCOMPtr<nsISettingsService> settingsService =
> +    do_GetService("@mozilla.org/settingsService;1");
> +  NS_ENSURE_TRUE_VOID(settingsService);
> +  nsCOMPtr<nsISettingsServiceLock> lock;
> +  settingsService->CreateLock(getter_AddRefs(lock));

Wonder if this can actually fail.. Other examples of CreateLock calls in gecko don't necessarily seem to check, though I'm not sure if that actually means it's ok.

Comment 4

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #3)
> if (!JSVAL_IS_INT(aResult))
>   return NS_OK;
> 
> or use NS_ENSURE_TRUE which would give us a warning. It seems like something
> would be very broken if we actually manage to hit that, so a warning would
> be nice.

I considered first option but didn't see great benefit on code architecture and just add more line.
But the second option looks more great so take it. Thanks.

> Why not NS_ConvertUTF16toUTF8? (not that it would matter much here)

I modified this.
 
> > +  settingsService->CreateLock(getter_AddRefs(lock));
> 

You are right. I added check for this and line as below.

> nsCOMPtr<nsISettingsServiceCallback> callback = new AudioChannelVolInitCallback();

Thanks.

Comment 5

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=c9e330736a42

Comment 6

[Marco Chen [:mchen]]

Attachment: bug-904531-AudioManager-init-vol-checkin

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3c1589a85de8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c1589a85de8