[Bug 732572]

[Karlt]

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?

-- 
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla.
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