1448914 - remove nsISound::playSystemSound

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Nathan Froyd [:froydnj]]

This API is deprecated (callers are supposed to be using
nsISound::playEventSound instead) and there are no callers remaining in
mozilla-central, or references to the strings for the API.  Let's remove
dead code.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: remove nsISound::playSystemSound

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It's used only by comm-central, but the path is fallback path and it's called with empty string. So, playSystemSound must do nothing. I think that it's fine to remove this API.
https://searchfox.org/comm-central/source/mailnews/base/src/nsStatusBarBiffManager.cpp#142

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> It's used only by comm-central, but the path is fallback path and it's
> called with empty string. So, playSystemSound must do nothing. I think that
> it's fine to remove this API.
What makes you think that it's empty?
        NS_ConvertUTF8toUTF16 utf16SoundURLSpec(soundURLSpec);
        rv = mSound->PlaySystemSound(utf16SoundURLSpec);
Doesn't look so empty to me.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b858515d3111
remove nsISound::playSystemSound; r=masayuki

Comment 5

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b858515d3111

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+1) from comment #3)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> > It's used only by comm-central, but the path is fallback path and it's
> > called with empty string. So, playSystemSound must do nothing. I think that
> > it's fine to remove this API.
> What makes you think that it's empty?
>         NS_ConvertUTF8toUTF16 utf16SoundURLSpec(soundURLSpec);
>         rv = mSound->PlaySystemSound(utf16SoundURLSpec);
> Doesn't look so empty to me.

Ah, I misunderstood the check of |soundURLSpec.IsEmpty()|. I thought it's the if block of the else block containing mSound->PlaySystemSound.

Comment 7

[Jorg K (CEST = GMT+2)]

I think this didn't work in the first place since to trigger system sounds you would have had to pass in magic constants like _moz_mailbeep, etc.:
https://dxr.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af/widget/nsISound.idl#55

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+1) from comment #7)
> I think this didn't work in the first place since to trigger system sounds
> you would have had to pass in magic constants like _moz_mailbeep, etc.:
> https://dxr.mozilla.org/mozilla-central/rev/
> de32269720d056972b85f4eec5f0a8286de6e3af/widget/nsISound.idl#55

Yeah. So, if it's necessary, map pref value to nsISound.EVENT_* and call playEventSound() for keep supporting the sounds named with the magic constants.

Or, does Thunderbird need to support some OS specific sound name like "Basso" of macOS? Perhaps, it's impossible to play such system sounds without nsISound.playSystemSound.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> Yeah. So, if it's necessary, map pref value to nsISound.EVENT_* and call
> playEventSound() for keep supporting the sounds named with the magic
> constants.
Yes, but I don't think anyone used the magic constants.

> Or, does Thunderbird need to support some OS specific sound name like
> "Basso" of macOS? Perhaps, it's impossible to play such system sounds
> without nsISound.playSystemSound.
Hmm, looks like it, but you dropped it, so what do we do :-(
https://hg.mozilla.org/mozilla-central/rev/b858515d3111#l1.22

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+1) from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> > Or, does Thunderbird need to support some OS specific sound name like
> > "Basso" of macOS? Perhaps, it's impossible to play such system sounds
> > without nsISound.playSystemSound.
> Hmm, looks like it, but you dropped it, so what do we do :-(
> https://hg.mozilla.org/mozilla-central/rev/b858515d3111#l1.22

If you need to access such special system sound, we could use nsISound.play() with defining special URI. Like https://system-sound.mozilla.org/*. Anyway, if you need, let me know.