503248 - nsISound::playSystemSound("_moz_mailbeep"); is obsolete on Gecko1.9.2a1 and later

Status: RESOLVED FIXED

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

|nsISound::playSystemSound("_moz_mailbeep");| is obsolete on Gecko1.9.2a1 and
later. Use |nsISound::playEventSound(nsISound::EVENT_NEW_MAIL_RECIEVED);|
instead.

This is not a bug for *current* SeaMonkey trunk build. See also bug 503246.

http://mxr.mozilla.org/comm-central/source/suite/mailnews/prefs/pref-notifications.js#118

Comment 1

[Robert Kaiser]

Does the new thing work on 1.9.1 as well or is this an either-or situation? If so, we probably need to introduce an ifdef while we're building with both 1.9.1 and trunk from the same comm-central tree.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

(In reply to comment #1)
> Does the new thing work on 1.9.1 as well or is this an either-or situation? If
> so, we probably need to introduce an ifdef while we're building with both 1.9.1
> and trunk from the same comm-central tree.

The new API isn't on 1.9.1 branch. So, if we support both trunk and 1.9.1 branch from same comm-central code, we need ifdef, so, you're right, unfortunately.

Comment 4

[Robert Kaiser]

Hmm, it looks like there's still a case in core widgets where this is found: http://mxr.mozilla.org/comm-central/search?string=_moz_mailbeep

What's the story on this nowadays?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

nsISound definition is for compatibility.

And others are not in core.

Comment 6

[Robert Kaiser]

OK, so both Thunderbird and SeaMonkey should switch away from this, right?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Right.

Comment 8

[Edmund Wong (:ewong)]

Attachment: use nsISound::EVENT_NEW_MAIL_RECEIVED if available.

Comment 9

[Ian Neal]

Comment on attachment 537714 [details] [diff] [review]
use nsISound::EVENT_NEW_MAIL_RECEIVED if available.

Review of attachment 537714 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/mailnews/prefs/pref-notifications.js
@@ +118,5 @@
> +  {
> +    if ('EVENT_NEW_MAIL_RECEIVED' in Components.interfaces.nsISound)
> +      gSound.playEventSound(nsISound::EVENT_NEW_MAIL_RECEIVED);
> +    else
> +      gSound.playSystemSound("_moz_mailbeep");

Isn't the if always going to be true?
Have you actually tested this patch? The syntax for the argument in playEventSound is currently in C++ not JS language.
We need to move away from _moz_mailbeep completely, so just beep here - see one of the other pref panels where we play a sound.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Replace playSystemSound("_moz_mailbeep");

Comment 11

[Ian Neal]

Comment on attachment 538175 [details] [diff] [review]
Replace playSystemSound("_moz_mailbeep");

There is still the issue of what to do on OSX which doesn't have any event sounds.
I would check if the platform was a Mac and do a beep for that.

Comment 12

[Edmund Wong (:ewong)]

Attachment: Replace nsISound::playSystemSound("_moz_mailbeep")

Comment 13

[Philip Chee]

> +  var Cin = Components.interfaces.nsISound;
Suite convention is: |var nsISound|

>    if (soundURL)
>      gSound.play(gIOService.newURI(soundURL, null, null));
>    else
> -    gSound.playSystemSound("_moz_mailbeep");
> +  {
> +    if (/Mac/.test(navigator.platform))
> +      gSound.Beep();
> +    else
> +      gSound.playEventSound(Cin.EVENT_NEW_MAIL_RECEIVED);
> +  }
I don't think you need to nest the if/elses.
if (soundURL)
  gSound.play(gIOService.newURI(soundURL, null, null));
else if (/Mac/.test(navigator.platform))
  gSound.Beep();
else
  gSound.playEventSound(nsISound.EVENT_NEW_MAIL_RECEIVED);

Comment 14

[Stefan [:stefanh]]

Comment on attachment 538762 [details] [diff] [review]
Replace nsISound::playSystemSound("_moz_mailbeep")

Drive-by comments:

+  var Cin = Components.interfaces.nsISound;
I don't believe you gain anything with this. It's just an extra variable and it might in fact not even be needed (if we have gSound).

+      gSound.Beep();
You missed my IRC comment ;-) This should be "gSound.beep();"

Comment 15

[Edmund Wong (:ewong)]

Attachment: Replace nsISound::playSystemSound("_moz_mailbeep") (v3)

Comment 16

[Edmund Wong (:ewong)]

Attachment: Replace nsISound::playSystemSound("_moz_mailbeep") (v3)

Comment 17

[Ian Neal]

Comment on attachment 538869 [details] [diff] [review]
Replace nsISound::playSystemSound("_moz_mailbeep") (v3)

I would prefer to have a:
const nsISound = Components.interfaces.nsISound;
at the beginning and then use nsISound in the two lines.
r=me with that fixed.

Comment 18

[Edmund Wong (:ewong)]

Attachment: Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: trunk Comment 20, aurora & beta Comment 23 ]

Changed according to comment #17.

Comment 19

[Philip Chee]

I think IanN meant at the beginning of the function, rather than at the beginning of the file.

Comment 20

[Ian Neal]

Comment on attachment 539522 [details] [diff] [review]
Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: trunk Comment 20, aurora & beta Comment 23 ]

Checked in with const in correct place
http://hg.mozilla.org/comm-central/rev/ed07dea78b29

Comment 21

[Ian Neal]

Comment on attachment 539522 [details] [diff] [review]
Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: trunk Comment 20, aurora & beta Comment 23 ]

This should probably be fixed on 2.2 and 2.3 as well using changeset from http://hg.mozilla.org/comm-central/rev/ed07dea78b29
Low risk and fixes an issue on OSX.

Comment 22

[Ian Neal]

(In reply to comment #21)
> Comment on attachment 539522 [details] [diff] [review] [review]
> Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: Comment
> 20]
> 
> This should probably be fixed on 2.2 and 2.3 as well using changeset from
> http://hg.mozilla.org/comm-central/rev/ed07dea78b29
> Low risk and fixes an issue on OSX.

(Saying that it's not gone only depreciated, so we could get away with not having it on 2.2 and 2.3)

Comment 23

[Ian Neal]

Comment on attachment 539522 [details] [diff] [review]
Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: trunk Comment 20, aurora & beta Comment 23 ]

http://hg.mozilla.org/releases/comm-aurora/rev/5af4d0099eaa
http://hg.mozilla.org/releases/comm-beta/rev/54a07d87ea36