desktop-packages team mailing list archive

[Chris Coulson <chris.coulson@xxxxxxxxxxxxx>]

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

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