1141932 - Chat sound preview doesn't work for default system sound on Mac OS

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

No sound is heard when the chat sound preview button is pressed for default system sound on Mac OS.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsISound#playSystemSound() suggests that playSystemSound isn't supported on Macintosh.

So, this is a possible fix. It fixes the issue for me.

Thanks.

Comment 1

[aleth [:aleth]]

Comment on attachment 8575800 [details] [diff] [review]
Patch v1

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

::: mail/components/preferences/chat.js
@@ +60,5 @@
>      let soundLocation = document.getElementById("chatSoundType").value == 1 ?
>                          document.getElementById("chatSoundUrlLocation").value :
>                          "_moz_mailbeep";
>  
>      if (!soundLocation.startsWith("file://"))

Please a add a comment saying that this should match the code in nsStatusBarBiffManager::PlayBiffSound.

@@ +68,1 @@
>        sound.playSystemSound(soundLocation);

This does not seem to match what actually happens for messages, nsStatusBarBiffManager:PlayBiffSound uses playEventSound

Comment 2

[Suyash Agarwal (:sshagarwal)]

(In reply to aleth [:aleth] from comment #1)
> Comment on attachment 8575800 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8575800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/preferences/chat.js
> @@ +60,5 @@
> >      let soundLocation = document.getElementById("chatSoundType").value == 1 ?
> >                          document.getElementById("chatSoundUrlLocation").value :
> >                          "_moz_mailbeep";
> >  
> >      if (!soundLocation.startsWith("file://"))
> 
> Please a add a comment saying that this should match the code in
> nsStatusBarBiffManager::PlayBiffSound.
> 
> @@ +68,1 @@
> >        sound.playSystemSound(soundLocation);
> 
> This does not seem to match what actually happens for messages,
> nsStatusBarBiffManager:PlayBiffSound uses playEventSound

Okay, will replace this call as well.
So, I think we are getting rid of "_moz_mailbeep" altogether then.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Okay so I have removed "_moz_mailbeep" as it wasn't being used anywhere now.
Also, I was told that using heterogenous type of date for the same variable, though allowed, isn't a good idea for many reasons and the test of soundLocation.startsWith would have failed, I didn't assign nsISound.EVENT.. to the soundLocation as a part of the ternary operator as left it blank.

Thanks.

Comment 4

[aleth [:aleth]]

Comment on attachment 8576583 [details] [diff] [review]
Patch v2

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

Thanks!

Please add the comment I requested before you land this.

Comment 5

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Okay, thanks.

If this comment isn't correct, please let me know.

Comment 6

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/b476a043292b