469164 - Implement nsSound::PlaySystemSound of Cocoa

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Attachment: Patch v1.0

This patch implements nsSound::PlaySystemSound of cocoa. However, Mac users cannot define which sound is used when what event like Windows. Therefore, we should give the definition way in our prefs. However, the way is very useful for other platforms too. Because some sound events are not defined on the platform, but users may want to play a sound on some events. So, this patch may be a help for some accessibility issues.

the pref names are:

"ui.systemsound." + NS_SYSTEMSOUND_* (without "_moz_" prefix)

E.g., NS_SYSSOUND_MAIL_BEEP is "_moz_mailbeep". Then, the pref name is "ui.systemsound.mailbeep".

The value of the prefs depends on the platform. It can be empty string or "-moz-system-beep" or param for the system API.

If it is empty string, we don't play any sounds. If it is "-moz-system-beep", we play just beep. Otherwise we call system sound playing API with the value.

The all pref values on Mac are empty. I'm not sure the suitable values for them on Mac. Please tell me, if you have good idea.

And also this patch helps bug 74628 and Linux distributors, probably.

Comment 1

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

(In reply to comment #0)
> And also this patch helps bug 74628 and Linux distributors, probably.

Oops, I meant bug 419275. But the bug has a patch and it passed reviews! I'll update my patch after bug 419275!

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not convinced we really need this. Mac doesn't have any of these system sounds as far as I know. The extra level of configurability might be useful, but has anyone actually asked for it?

Comment 3

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

I think that this patch is useful for XUL applications. XUL applications can override the system sounds by their pref files (e.g., mailnews.js). Then, even if the OS doesn't have system OS for some events, XUL application can play beep or sounds. E.g., Thunderbird on Mac has "System Alert Sound" pref in its preference dialog for new message arrive. However, thunderbird cannot play any sounds for this setting now. But this patch helps to enable this setting.

And also on Linux, some distributors may want to customize Fx/Tb system sounds. Then they can edit only prefs for this case. (but I'm not sure whether such case is there.)

And some Japanese bloggers worry the non-configurability about alert sounds on Win32. E.g., some Windows users may choose Fx by the silence of the alerts. Then, we lost such users. (I don't have this worry. Because such users should disable the system sounds already on Control Panel, but some people don't think so...)

This patch can help to create new system sound support without native code changing. It might helps some addon developers.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If XUL applications want to play specific sounds, they can use nsISound or <audio> elements to do so. The only extra functionality this adds for XUL applications is the ability to specify sounds that are played for things like JS alerts where the application isn't in control. That doesn't seem important.

I don't see why Linux distributors would want to make Firefox less like other applications.

Realistically very few users would configure their own sounds via about:config. We shouldn't be adding code just for that.

Comment 5

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

(In reply to comment #4)
> I don't see why Linux distributors would want to make Firefox less like other
> applications.

The Linux and Gnome release timing and our product release timing are not sync. So, if some sound events are added to Gnome, then, the distributors can support the new events on Fx if the events are already supported on Fx for other platforms.

But I'm not familiar for Linux distributions and Gnome, therefore I'm not sure whether this feature is important for them. But if needed, it should be separated to another bug now.

ok. I'll post new patch which only changes nsSound of cocoa.

Comment 6

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

Attachment: Patch v2.0

The previous patch has wrong code. nsSound::PlaySystemSound should not treat the param as file path. It should be sound name always.

Comment 7

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

Ah, you can test this patch in input field of Error Console. Type:

Components.Constructor("@mozilla.org/sound;1","nsISound")().playSystemSound("Blow");

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #5)
> (In reply to comment #4)
> > I don't see why Linux distributors would want to make Firefox less like other
> > applications.
> 
> The Linux and Gnome release timing and our product release timing are not sync.
> So, if some sound events are added to Gnome, then, the distributors can support
> the new events on Fx if the events are already supported on Fx for other
> platforms.

They can just patch their Firefox build to support the new sounds.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do we need to free the sound and the name?

Comment 10

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

(In reply to comment #9)
> Do we need to free the sound and the name?

No. We must not free them. sound is global object, if we free it, we cannot replay the sound. If we free name, it will be re-freed and hanging up.

Comment 11

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

Attachment: Patch v2.1

Oh, this is better.

  NSSound *sound = [NSSound soundNamed:name];
  if (sound) {
+   [sound stop];
    [sound play];
  }

If the same system sound is playing, we cannot replay the sound until it is finished. Then, the newer play command is discarded. So, I think that we should stop the previous system sound playing and replay it from start.

Comment 12

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

Comment on attachment 352825 [details] [diff] [review]
Patch v2.1

Steven, could you review this?

Comment 13

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

Comment on attachment 352825 [details] [diff] [review]
Patch v2.1

Thank you josh for your review.

Comment 14

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

pushed to trunk.
http://hg.mozilla.org/mozilla-central/rev/795d837f70ac

Comment 15

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

Comment on attachment 352825 [details] [diff] [review]
Patch v2.1

Fx doesn't play sounds by this patch. Because Mac doesn't have sound settings for user actions and system events. So, the risk of this patch is low.

But this patch makes that add-on developers and other XUL apps can play system sounds of Mac by specifying a system sound name directly. So, some developers may want to use this API on their applications.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352825 [details] [diff] [review]
Patch v2.1

a191=beltzner

Comment 17

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ae55f1f28013