support for gnome's sounds

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Gtk
--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Jakub 'Livio' Rusinek, Assigned: Michael Ventnor)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {fixed1.9.1})

unspecified
mozilla1.9.2a1
x86
Linux
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; pl; rv:1.9b4pre) Gecko/2008022304 Fedora/8 (Werewolf) Minefield/3.0b4pre
Build Identifier: 

firefox should support gnome's sounds through its api or something like that.

I need some attention when message is shown.

Reproducible: Always

Actual Results:  
no sounds

Expected Results:  
sounds

not sure where this should land.

Updated

9 years ago
Blocks: 461963
(Assignee)

Comment 1

9 years ago
Taking, will implement by dynamically linking libcanberra.
Assignee: nobody → ventnor.bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

9 years ago
Created attachment 352201 [details] [diff] [review]
Patch

Seems to work. No crashes so far.
Attachment #352201 - Flags: superreview?(roc)
Attachment #352201 - Flags: review?(roc)
(Assignee)

Comment 3

9 years ago
Actually, its probably better I avoided the use of static privates and just made a new context for each event sound anyway, shouldn't be too bad on perf.
(Assignee)

Comment 4

9 years ago
Actually, my mistake. I remembered why I'm using that now, libcanberra spawns a new thread.
+/* used to find and play common system event sounds.
+   this interfaces with libcanberra.
+ */
+typedef struct _ca_context ca_context;

Is it worth copying this stuff from libcanberra, or should we just have a configure check so that this code is only built when libcanberra dev headers are installed?

+    if (g_object_class_find_property(G_OBJECT_GET_CLASS(settings), "gtk-sound-theme-name") &&
+        g_object_class_find_property(G_OBJECT_GET_CLASS(settings), "gtk-enable-event-sounds")) {

Should we bail out if these properties are not found?

+        if (!ctx)
+            return NS_ERROR_OUT_OF_MEMORY;

You're leaking sound_theme_name here. Maybe move the context-getting code further up near the top of the function?

+        ca_context_change_props(ctx, "canberra.xdg-theme.name", sound_theme_name, NULL);

Why are we responsible for this? Can we do it just once or do we really need to do it every time?
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> +/* used to find and play common system event sounds.
> +   this interfaces with libcanberra.
> + */
> +typedef struct _ca_context ca_context;
> 
> Is it worth copying this stuff from libcanberra, or should we just have a
> configure check so that this code is only built when libcanberra dev headers
> are installed?

Definitely worth it, maybe reconsider at a later stage. libcanberra is very new, only GNOME 2.24 has it as a new dependency.

> +    if (g_object_class_find_property(G_OBJECT_GET_CLASS(settings),
> "gtk-sound-theme-name") &&
> +        g_object_class_find_property(G_OBJECT_GET_CLASS(settings),
> "gtk-enable-event-sounds")) {
> 
> Should we bail out if these properties are not found?

No, this is from latest stable GTK. A person could still have libcanberra installed but an older GTK in which we should and would use the sound theme "freedesktop" by default.

> +        if (!ctx)
> +            return NS_ERROR_OUT_OF_MEMORY;
> 
> You're leaking sound_theme_name here. Maybe move the context-getting code
> further up near the top of the function?

OK.

> +        ca_context_change_props(ctx, "canberra.xdg-theme.name",
> sound_theme_name, NULL);
> 
> Why are we responsible for this? Can we do it just once or do we really need to
> do it every time?

Well, if we do it every time we ensure we keep our sound theme selection up to date if the user changes it while Firefox is running. If we don't do this we'll always play the generic "freedesktop" theme.
(Assignee)

Comment 7

9 years ago
(In reply to comment #5)
> +        if (!ctx)
> +            return NS_ERROR_OUT_OF_MEMORY;
> 
> You're leaking sound_theme_name here. Maybe move the context-getting code
> further up near the top of the function?

Or I could add a further free() check there. The reason I moved the code there was because if sound themes were disabled we don't unnecessarily create and throw out a canberra context.
(Assignee)

Comment 8

9 years ago
Created attachment 352217 [details] [diff] [review]
Patch 2

Nah, I'll just move the code.
Attachment #352201 - Attachment is obsolete: true
Attachment #352217 - Flags: superreview?(roc)
Attachment #352217 - Flags: review?(roc)
Attachment #352201 - Flags: superreview?(roc)
Attachment #352201 - Flags: review?(roc)
No, you convinced me in comment #7 that the code should stay down there so we don't create a libcanberra context unnecessarily. Go back to that and add the free() and we're good.
(Assignee)

Comment 10

9 years ago
Created attachment 352220 [details] [diff] [review]
Patch 3

Concerned about unnecessary work? Who are you and what have you done with the real roc?
Attachment #352217 - Attachment is obsolete: true
Attachment #352220 - Flags: superreview?(roc)
Attachment #352220 - Flags: review?(roc)
Attachment #352217 - Flags: superreview?(roc)
Attachment #352217 - Flags: review?(roc)
+            if (sound_theme_name)
+                g_free(sound_theme_name);

Isn't g_free NULL safe? You should just be able to do g_free here without the if.

(In reply to comment #10)
> Concerned about unnecessary work? Who are you and what have you done with the
> real roc?

An extra thread is significant enough overhead that it's worth avoiding.
(Assignee)

Comment 12

9 years ago
Created attachment 352227 [details] [diff] [review]
Patch 4

Oh, I didn't know g_free was NULL-safe.
Attachment #352220 - Attachment is obsolete: true
Attachment #352227 - Flags: superreview?(roc)
Attachment #352227 - Flags: review?(roc)
Attachment #352220 - Flags: superreview?(roc)
Attachment #352220 - Flags: review?(roc)
Attachment #352227 - Flags: superreview?(roc)
Attachment #352227 - Flags: superreview+
Attachment #352227 - Flags: review?(roc)
Attachment #352227 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/a873ac255439
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

9 years ago
Attachment #352227 - Flags: approval1.9.1?
(Assignee)

Comment 14

9 years ago
Comment on attachment 352227 [details] [diff] [review]
Patch 4

The original patch made it into 1.9.1. I think this should too, especially the benefit this will bring for Thunderbird (getting rid of that awful beep too).
Yeah, I'd like to see this in 1.9.1 too. The risk is very low.
> +nsresult nsSound::PlaySystemEventSound(const nsAString &aSoundAlias)
> +{
> +    if (!libcanberra)
> +        return NS_ERROR_FAILURE;

Probably, this is not good. If libcanberra.so is not installed, nsISound::playSystemSound returns error. Then, the caller javascript will abort if the caller doesn't use try-catch. Some addons and tb/seamonkey may not work fine *only* on some Linux environments by this change.
(Assignee)

Comment 17

9 years ago
Yeah, I suppose that wasn't the brightest idea considering a lot of users don't in fact use a try block.
Michael, please post a followup patch to fix that.
(Assignee)

Updated

9 years ago
Attachment #352227 - Flags: approval1.9.1?
(Assignee)

Comment 19

9 years ago
Created attachment 352952 [details] [diff] [review]
Followup

Fixes the return code.
Attachment #352952 - Flags: superreview?(roc)
Attachment #352952 - Flags: review?(roc)
Attachment #352952 - Flags: superreview?(roc)
Attachment #352952 - Flags: superreview+
Attachment #352952 - Flags: review?(roc)
Attachment #352952 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs landing] followup needs checkin
http://hg.mozilla.org/mozilla-central/rev/08d1e9d006a5
Keywords: checkin-needed
Whiteboard: [needs landing] followup needs checkin

Comment 21

9 years ago
This appears to be causing random oranges on Linux tinderboxes:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229140245.1229142959.23926.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229322185.1229324135.13138.gz

The errors are, I assume, caused by lack of a sound card in the tinderbox machine.

These errors are similar to ones I use to get in the video code that used the libsydneyaudio alsa backend. The alsa library outputs error messages in a 'chatty' fashion and we had to turn them off in a change to libsydneyaudio.

Updated

9 years ago
Blocks: 469635

Comment 22

9 years ago
Raised bug 469635 for the issue in comment 21.
(Assignee)

Comment 23

9 years ago
Created attachment 353141 [details] [diff] [review]
Combined patch

Contains the patch and followup for 1.9.1 approval request.
Attachment #353141 - Flags: approval1.9.1?
Comment on attachment 353141 [details] [diff] [review]
Combined patch

a191=beltzner
Attachment #353141 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 landing]
No longer blocks: 469635
Depends on: 469635
Pushed f1449dc05aff to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
(Also pushed the followup as 61bd39f25e6f)
(In reply to comment #14)
> (From update of attachment 352227 [details] [diff] [review])
> The original patch made it into 1.9.1. I think this should too, especially the
> benefit this will bring for Thunderbird (getting rid of that awful beep too).

Doesn't this patch also get rid of that awful beep for people without libcanberra?  We never call ::Beep() anymore with this patch...
Depends on: 504670
You need to log in before you can comment on or make changes to this bug.