Closed
Bug 503248
Opened 13 years ago
Closed 11 years ago
nsISound::playSystemSound("_moz_mailbeep"); is obsolete on Gecko1.9.2a1 and later
Categories
(SeaMonkey :: OS Integration, defect)
SeaMonkey
OS Integration
Tracking
(seamonkey2.2 fixed, seamonkey2.3 fixed)
RESOLVED
FIXED
seamonkey2.2
People
(Reporter: masayuki, Assigned: ewong)
References
Details
Attachments
(1 file, 5 obsolete files)
1.49 KB,
patch
|
ewong
:
review+
kairo
:
approval-comm-aurora+
kairo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
|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•13 years ago
|
||
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.
Reporter | ||
Comment 3•13 years ago
|
||
(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•12 years ago
|
||
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?
Reporter | ||
Comment 5•12 years ago
|
||
nsISound definition is for compatibility. And others are not in core.
![]() |
||
Comment 6•12 years ago
|
||
OK, so both Thunderbird and SeaMonkey should switch away from this, right?
Reporter | ||
Comment 7•12 years ago
|
||
Right.
![]() |
Assignee | |
Updated•11 years ago
|
Assignee: nobody → ewong
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 8•11 years ago
|
||
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-
![]() |
Assignee | |
Comment 10•11 years ago
|
||
Attachment #537714 -
Attachment is obsolete: true
Attachment #538175 -
Flags: review?(iann_bugzilla)
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #538175 -
Flags: review?(iann_bugzilla)
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #538175 -
Flags: review?(iann_bugzilla)
Comment 11•11 years ago
|
||
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-
![]() |
Assignee | |
Comment 12•11 years ago
|
||
Attachment #538175 -
Attachment is obsolete: true
Attachment #538762 -
Flags: review?(iann_bugzilla)
![]() |
||
Comment 13•11 years ago
|
||
> + 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•11 years ago
|
||
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();"
![]() |
Assignee | |
Comment 15•11 years ago
|
||
Attachment #538762 -
Attachment is obsolete: true
Attachment #538868 -
Flags: review?(iann_bugzilla)
Attachment #538762 -
Flags: review?(iann_bugzilla)
![]() |
Assignee | |
Comment 16•11 years ago
|
||
Attachment #538868 -
Attachment is obsolete: true
Attachment #538869 -
Flags: review?(iann_bugzilla)
Attachment #538868 -
Flags: review?(iann_bugzilla)
Comment 17•11 years ago
|
||
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+
![]() |
Assignee | |
Comment 18•11 years ago
|
||
Changed according to comment #17.
Attachment #538869 -
Attachment is obsolete: true
Attachment #539522 -
Flags: review+
![]() |
Assignee | |
Updated•11 years ago
|
Keywords: checkin-needed
![]() |
Assignee | |
Updated•11 years ago
|
Keywords: checkin-needed
![]() |
||
Comment 19•11 years ago
|
||
I think IanN meant at the beginning of the function, rather than at the beginning of the file.
Comment 20•11 years ago
|
||
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: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.4
Comment 21•11 years ago
|
||
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?
Comment 22•11 years ago
|
||
(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)
![]() |
||
Updated•11 years ago
|
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 23•11 years ago
|
||
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 ]
Updated•11 years ago
|
status-seamonkey2.2:
--- → fixed
status-seamonkey2.3:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•