[Bug 732572]

[Matthew Gregan]

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.

-- 
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