desktop-packages team mailing list archive

[Matthew Gregan <kinetik@xxxxxxxx>]

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 Desktop
Packages, which is subscribed to thunderbird in Ubuntu.
https://bugs.launchpad.net/bugs/732572

Title:
  New Mail Notification Sound does not play in Natty

Status in Mozilla Thunderbird Mail and News:
  In Progress
Status in “thunderbird” package in Ubuntu:
  Triaged
Status in “thunderbird” source package in Oneiric:
  Triaged

Bug description:
  Binary package hint: thunderbird

  After setting the preference to play a .wav file and selecting a working .wav file, it will not play by ether:
  1. Clicking the Play button in the preference dialog
  2. Receiving a new email

  Expected result is that it should play the selected file.

  ProblemType: Bug
  DistroRelease: Ubuntu 11.04
  Package: thunderbird 3.1.9+nobinonly-0ubuntu1
  ProcVersionSignature: Ubuntu 2.6.38-5.32-generic-pae 2.6.38-rc6
  Uname: Linux 2.6.38-5-generic-pae i686
  Architecture: i386
  Date: Thu Mar 10 13:23:55 2011
  EcryptfsInUse: Yes
  InstallationMedia: Ubuntu 11.04 "Natty Narwhal" - Alpha i386 (20110302)
  ProcEnviron:
   LANGUAGE=en_US:en
   PATH=(custom, user)
   LANG=en_US.UTF-8
   SHELL=/bin/bash
  SourcePackage: thunderbird
  UpgradeStatus: No upgrade log present (probably fresh install)

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/732572/+subscriptions