Comment 38 for bug 732572

Comment on attachment 566854
Bug 635918 Part 1 - Make nsISound::Play use libcanberra on Linux rather than esound

Review of attachment 566854:
-----------------------------------------------------------------

This looks great, thank you. Removing esd makes me very happy.

I'm adding karlt to feedback? to get a second set of eyes on this.

::: widget/src/gtk2/nsSound.cpp
@@ +65,1 @@
> #define WAV_MIN_LENGTH 44

This can be removed now.

@@ +114,5 @@
>
> +typedef struct {
> + PRFileDesc *mFD;
> + nsCString mPath;
> +} CanberraPlayCBData;

No need for a typedef, this is C++. More comments on CanberyyaPlayCBData below.

@@ +167,5 @@
> + uint32_t id,
> + int error_code,
> + void *userdata)
> +{
> + CanberraPlayCBData *data = (CanberraPlayCBData *) userdata;

reinterpret_cast please.

@@ +203,1 @@
> if (!libasound) {

All of the libasound stuff can be removed. Its only purpose was to install a silent error handler on behalf of esound, which is no longer necessary.

@@ +251,5 @@
> nsresult aStatus,
> PRUint32 dataLen,
> const PRUint8 *data)
> {
> + nsresult rv;

Declare this where it's first used.

@@ +334,4 @@
>
> + CanberraPlayCBData *cbdata = new CanberraPlayCBData();
> + cbdata->mFD = fd;
> + cbdata->mPath = path;

Add a constructor, this could then be:
  CanberraPlayCBData *cbdata = new CanberraPlayCBData(fd, path);

It might be better to wrap all of this in an nsAutoRef so that you don't need to remember to PR_Delete/PR_Close in every error path.

@@ +375,5 @@
> +
> + ca_context_play(ctx, 0, "media.filename", path.get(), NULL);
> + } else {
> + nsCOMPtr<nsIStreamLoader> loader;
> + rv = NS_NewStreamLoader(getter_AddRefs(loader), aURL, this);

rv isn't checked.