Last Comment Bug 419275 - support for gnome's sounds
: support for gnome's sounds
Status: RESOLVED FIXED
: fixed1.9.1
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: unspecified
: x86 Linux
: -- enhancement (vote)
: mozilla1.9.2a1
Assigned To: Michael Ventnor
:
Mentors:
Depends on: 504670 469635
Blocks: 461963
  Show dependency treegraph
 
Reported: 2008-02-24 03:37 PST by Jakub 'Livio' Rusinek
Modified: 2009-07-30 16:12 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.50 KB, patch)
2008-12-09 14:55 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 (6.43 KB, patch)
2008-12-09 16:13 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 3 (6.59 KB, patch)
2008-12-09 16:29 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 4 (6.51 KB, patch)
2008-12-09 16:54 PST, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Followup (398 bytes, patch)
2008-12-14 17:13 PST, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Combined patch (6.50 KB, patch)
2008-12-15 17:51 PST, Michael Ventnor
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description Jakub 'Livio' Rusinek 2008-02-24 03:37:57 PST
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.
Comment 1 Michael Ventnor 2008-12-09 14:54:26 PST
Taking, will implement by dynamically linking libcanberra.
Comment 2 Michael Ventnor 2008-12-09 14:55:18 PST
Created attachment 352201 [details] [diff] [review]
Patch

Seems to work. No crashes so far.
Comment 3 Michael Ventnor 2008-12-09 15:07:40 PST
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.
Comment 4 Michael Ventnor 2008-12-09 15:09:28 PST
Actually, my mistake. I remembered why I'm using that now, libcanberra spawns a new thread.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-09 15:36:52 PST
+/* 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?
Comment 6 Michael Ventnor 2008-12-09 15:49:23 PST
(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.
Comment 7 Michael Ventnor 2008-12-09 15:50:43 PST
(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.
Comment 8 Michael Ventnor 2008-12-09 16:13:02 PST
Created attachment 352217 [details] [diff] [review]
Patch 2

Nah, I'll just move the code.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-09 16:18:44 PST
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.
Comment 10 Michael Ventnor 2008-12-09 16:29:07 PST
Created attachment 352220 [details] [diff] [review]
Patch 3

Concerned about unnecessary work? Who are you and what have you done with the real roc?
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-09 16:33:37 PST
+            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.
Comment 12 Michael Ventnor 2008-12-09 16:54:13 PST
Created attachment 352227 [details] [diff] [review]
Patch 4

Oh, I didn't know g_free was NULL-safe.
Comment 13 Reed Loden [:reed] (use needinfo?) 2008-12-12 15:41:51 PST
http://hg.mozilla.org/mozilla-central/rev/a873ac255439
Comment 14 Michael Ventnor 2008-12-12 16:09:24 PST
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).
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-12 19:46:46 PST
Yeah, I'd like to see this in 1.9.1 too. The risk is very low.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-14 05:04:32 PST
> +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.
Comment 17 Michael Ventnor 2008-12-14 12:30:06 PST
Yeah, I suppose that wasn't the brightest idea considering a lot of users don't in fact use a try block.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-14 13:38:44 PST
Michael, please post a followup patch to fix that.
Comment 19 Michael Ventnor 2008-12-14 17:13:55 PST
Created attachment 352952 [details] [diff] [review]
Followup

Fixes the return code.
Comment 20 Reed Loden [:reed] (use needinfo?) 2008-12-14 22:50:26 PST
http://hg.mozilla.org/mozilla-central/rev/08d1e9d006a5
Comment 21 cajbir (:cajbir) 2008-12-14 23:32:13 PST
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.
Comment 22 cajbir (:cajbir) 2008-12-14 23:39:18 PST
Raised bug 469635 for the issue in comment 21.
Comment 23 Michael Ventnor 2008-12-15 17:51:29 PST
Created attachment 353141 [details] [diff] [review]
Combined patch

Contains the patch and followup for 1.9.1 approval request.
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2008-12-16 05:50:37 PST
Comment on attachment 353141 [details] [diff] [review]
Combined patch

a191=beltzner
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-08 18:12:22 PST
Pushed f1449dc05aff to 1.9.1
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-08 18:14:23 PST
(Also pushed the followup as 61bd39f25e6f)
Comment 27 Matthew Gregan [:kinetik] 2009-02-23 11:34:07 PST
(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...

Note You need to log in before you can comment on or make changes to this bug.