Comment 43 for bug 732572

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