Comment 42 for bug 732572

Comment on attachment 570562
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

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.

> /* 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.

>- if (!elib)
>- return NS_ERROR_NOT_AVAILABLE;
>+ if (!libcanberra)
>+ return NS_OK;

Wouldn't NS_ERROR_NOT_AVAILABLE be more suitable here?