Closed Bug 1448914 Opened 2 years ago Closed 2 years ago

remove nsISound::playSystemSound

Categories

(Core :: Widget, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

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.
Attachment #8962386 - Flags: review?(masayuki)
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
Attachment #8962386 - Flags: review?(masayuki) → review+
(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.
Depends on: 1449124
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b858515d3111
remove nsISound::playSystemSound; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/b858515d3111
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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.
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
(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.
Flags: needinfo?(jorgk)
(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
Flags: needinfo?(jorgk)
(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.
Duplicate of this bug: 1430179
You need to log in before you can comment on or make changes to this bug.