Closed
Bug 419275
Opened 16 years ago
Closed 16 years ago
support for gnome's sounds
Categories
(Core :: Widget: Gtk, enhancement)
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)
6.51 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
398 bytes,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
6.50 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
Taking, will implement by dynamically linking libcanberra.
Assignee: nobody → ventnor.bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•16 years ago
|
||
Seems to work. No crashes so far.
Attachment #352201 -
Flags: superreview?(roc)
Attachment #352201 -
Flags: review?(roc)
Assignee | ||
Comment 3•16 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•16 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•16 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•16 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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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]
Comment 13•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Attachment #352227 -
Flags: approval1.9.1?
Assignee | ||
Comment 14•16 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.
Comment 16•16 years ago
|
||
> +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•16 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•16 years ago
|
Attachment #352227 -
Flags: approval1.9.1?
Assignee | ||
Comment 19•16 years ago
|
||
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•16 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing] followup needs checkin
Comment 20•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/08d1e9d006a5
Keywords: checkin-needed
Whiteboard: [needs landing] followup needs checkin
Comment 21•16 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.
Comment 22•16 years ago
|
||
Raised bug 469635 for the issue in comment 21.
Assignee | ||
Comment 23•16 years ago
|
||
Contains the patch and followup for 1.9.1 approval request.
Attachment #353141 -
Flags: approval1.9.1?
Comment 24•16 years ago
|
||
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)
Comment 27•15 years ago
|
||
(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...
You need to log in
before you can comment on or make changes to this bug.
Description
•