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

RESOLVED FIXED in seamonkey2.2

Status

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: masayuki, Assigned: ewong)

Tracking

Trunk
seamonkey2.2
Dependency tree / graph
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

(seamonkey2.2 fixed, seamonkey2.3 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

|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
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.
Duplicate of this bug: 503246
(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.
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?
nsISound definition is for compatibility.

And others are not in core.
OK, so both Thunderbird and SeaMonkey should switch away from this, right?
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #537714 - Flags: review?(iann_bugzilla)
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.
Attachment #537714 - Flags: review?(iann_bugzilla) → review-
Attachment #537714 - Attachment is obsolete: true
Attachment #538175 - Flags: review?(iann_bugzilla)
Attachment #538175 - Flags: review?(iann_bugzilla)
Attachment #538175 - Flags: review?(iann_bugzilla)
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.
Attachment #538175 - Flags: review?(iann_bugzilla) → review-
Attachment #538175 - Attachment is obsolete: true
Attachment #538762 - Flags: review?(iann_bugzilla)
> +  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 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();"
Attachment #538762 - Attachment is obsolete: true
Attachment #538868 - Flags: review?(iann_bugzilla)
Attachment #538762 - Flags: review?(iann_bugzilla)
Attachment #538868 - Attachment is obsolete: true
Attachment #538869 - Flags: review?(iann_bugzilla)
Attachment #538868 - Flags: review?(iann_bugzilla)
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.
Attachment #538869 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
Keywords: checkin-needed
I think IanN meant at the beginning of the function, rather than at the beginning of the file.
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
Attachment #539522 - Attachment description: Replace nsISound::playSystemSound("_moz_mailbeep") (v4) → Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: Comment 20]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.4
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.
Attachment #539522 - Flags: approval-comm-beta?
Attachment #539522 - Flags: approval-comm-aurora?
(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)
Attachment #539522 - Flags: approval-comm-beta?
Attachment #539522 - Flags: approval-comm-beta+
Attachment #539522 - Flags: approval-comm-aurora?
Attachment #539522 - Flags: approval-comm-aurora+
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
Attachment #539522 - Attachment description: Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: Comment 20] → Replace nsISound::playSystemSound("_moz_mailbeep") (v4) [Checked in: trunk Comment 20, aurora & beta Comment 23 ]
Target Milestone: seamonkey2.4 → seamonkey2.2
You need to log in before you can comment on or make changes to this bug.