remove nsISound::playSystemSound

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
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.
Assignee

Comment 1

a year ago
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

Comment 3

a year ago
(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.

Updated

a year ago
Depends on: 1449124

Comment 4

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b858515d3111
remove nsISound::playSystemSound; r=masayuki

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b858515d3111
Status: NEW → RESOLVED
Last Resolved: a year 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.

Comment 7

a year ago
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)

Comment 9

a year ago
(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.
You need to log in before you can comment on or make changes to this bug.