Closed Bug 419275 Opened 16 years ago Closed 16 years ago

support for gnome's sounds

Categories

(Core :: Widget: Gtk, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: u294409, Assigned: ventnor.bugzilla)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 3 obsolete files)

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.
Blocks: 461963
Taking, will implement by dynamically linking libcanberra.
Assignee: nobody → ventnor.bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
Seems to work. No crashes so far.
Attachment #352201 - Flags: superreview?(roc)
Attachment #352201 - Flags: review?(roc)
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.
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?
(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.
(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.
Attached patch Patch 2 (obsolete) — Splinter Review
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.
Attached patch Patch 3 (obsolete) — Splinter Review
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.
Attached patch Patch 4Splinter Review
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+
http://hg.mozilla.org/mozilla-central/rev/a873ac255439
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Attachment #352227 - Flags: approval1.9.1?
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.
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.
Attachment #352227 - Flags: approval1.9.1?
Attached patch FollowupSplinter Review
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+
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
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.
Blocks: 469635
Raised bug 469635 for the issue in comment 21.
Attached patch Combined patchSplinter Review
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]
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.

Attachment

General

Created:
Updated:
Size: