Created attachment 575899 Bug 635918 Part 1 - Make nsISound::Play use libcanberra on Linux rather than esound (v3)
(In reply to Karl Tomlinson (:karlt) from comment #30) Hi, thanks for the review. > Comment on attachment 570562 [diff] [details] [review] > Bug 635918 Part 1 - Make nsISound::Play use libcanberra on Linux rather than > esound (v2) > > Moving away from esound is looking very good, thanks. > > My main comment is that the file descriptor can be closed before > calling ca_context_play_full(), and before ca_context_get_default() even. > (I guess NSPR I/O is synchronous and unbuffered, so this may not be > essential, but there is no need to keep the descriptor open.) > > AutoFDClose exists for PRFileDesc, if that is useful. > http://hg.mozilla.org/mozilla-central/annotate/392fa68084a8/xpcom/glue/ > FileUtils.h#l55 > Ah, I wasn't aware of AutoFDClose. I've started using it now, so that the descriptor is just closed when OnStreamComplete is done (although, this still ends up being after ca_context_play_full, but it is before the canberra callback runs and does tidy up the various error paths) > I wondered about using nsI(Local)File::Remove() instead of PR_Delete() to get > rid of some NSPR usage and clear up the filename encoding expectations that > I'm not sure about. However, I assume the nsIFile can't be simply > ref-counted > on another thread and so that would all get complicated. > I've updated it to use nsILocalFile::Remove. I also just pass the nsILocalFile raw pointer as the callback data for ca_context_play_full now, and renamed CanberraPlayCBData to something more appropriate now (ScopedCanberraFile), as it only exists now to automatically remove the file when it goes out of scope. > > /* used to find and play common system event sounds. > > this interfaces with libcanberra. > > */ > > typedef struct _ca_context ca_context; > >+typedef struct _ca_proplist ca_proplist; > > I guess this comment could be updated, as this is not just system sounds now. > > >+ ca_context_play_full(ctx, 0, p, ca_finish_cb, cbdata); > > I assume the return code should be checked to avoid leaking when it fails. > Ok, I've fixed that too > >- if (!elib) > >- return NS_ERROR_NOT_AVAILABLE; > >+ if (!libcanberra) > >+ return NS_OK; > > Wouldn't NS_ERROR_NOT_AVAILABLE be more suitable here? Yeah, makes sense. Fixed as well -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/732572 Title: New Mail Notification Sound does not play in Natty To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/732572/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
