Bug 176602 replaced the typeahead find beep with a sound, enabled/disabled by user_pref("accessibility.typeaheadfind.enablesound", true); I've been thinking about how chatzilla and other apps handle sound: generally they let you choose a sound, with beep being an option. We could do the same thing as cz if the pref were changed to something like: user_pref("accessibility.typeaheadfind.sound", "url"); where url is the url to the sound (defaulting to the existing notfound.wav in the chrome jar), or "beep" to call the system beep (default on platforms where sound doesn't work), or "" or unset to disable sound. Then people on platforms where sound doesn't work could still have the original behavior, at their option.

just chatted w/akkana about this --she might have a fix for this which might resolve both bug 179817 and bug 179824.

Here's the general idea. I've only tested this very briefly, and it needs the default url added (on platforms that can use it) and the string removed from where it is now, but it does turn on beeping on unix if I set the pref to "beep". Can someone try mach-o?

Taking.

bryner or sfraser, would you be able to test/review this for mach-o, please?

Hmm, okay. I don't plan on exposing this in the prefs -- just a checkmark whether you want a sound or not there. How about a value of "default" meaning the sound we've checked in. Also, the all.js, unix.js etc. files would need to be changed.

and, of course, macprefs.js needs to be updated as well.

+ char *soundStr = 0; + prefs->CopyCharPref("accessibility.typeaheadfind.soundURL", &soundStr); + nsSharableCString soundURL; + if (soundStr) + soundURL.Adopt(soundStr); Why not use nsXPIDLCString? Looks ok otherwise, though I didn't test it in a build.

Aaron: good idea about "default". I've added that and set the all.js default to that, and added the unix.js default of "beep". I'm not sure what to do for macprefs.js, though. Should I set it to beep to fix the mach-o builds, thus depriving the carbon builds of the nicer sound? I don't see separate macho vs. carbon prefs files. I don't suppose macho builds pick up unix.js? Mac people, tell me what to do here. I didn't use nsXPIDLCString because all the CharPrefs examples I found used char*. I'll do a more extensive search and try to figure out how to use nsXPIDLCString, if that's the new approved way. I expect we'll need one more round here anyway, for adding the right macprefs.js setting.

macprefs.js affects both cfm and macho builds. unix.js is not used on Mac. I think we'd be fine if mac used "beep" everywhere; the .wav file sounds crappy anyway.

The wav file sounds a lot crappier on mac than on win for some reason. It has extra static. However, when we played the longer version of the wav file it sounds fine -- so we could still replace it with that. However, I'm not against using the beep on mac -- just wanted to clarify this.

Simon, how does this sound to you? http://bugzilla.mozilla.org/attachment.cgi?id=104093&action=view

Comment on attachment 106162 [details] [diff] [review] More complete pref: do everything but set the mac pref Looks nice, thanks for doing this. r=aaronl

my own preference would be that the pref for mac (which would affect both cfm and mach-o) be the same as for unix. the system beep on mac is easily configurable (eg, in the Sound system pref panel in OS X), so the user can make it as pleasant (or unpleasant) a s/he prefers.

That attachment sounds like a hoarse dog barking.

> That attachment sounds like a hoarse dog barking. Yeah, but that's better than a horse-dog.

+ char *soundStr = 0; + prefs->CopyCharPref("accessibility.typeaheadfind.soundURL", &soundStr); + nsSharableCString soundURL; + if (soundStr) + soundURL.Adopt(soundStr); + typeAheadFind->mNotFoundSoundURL = soundURL; This would go something like: nsXPIDLCString soundStr; prefs->CopyCharPref("accessibility.typeaheadfind.soundURL", getter_Copies(soundStr)); if (!soundStr.IsEmpty()) typeAheadFind->mNotFoundSoundURL = (!soundStr; + if (mNotFoundSoundURL.Equals("default")) Why not just put the default sound url in the default prefs, and remove this special "default" value? +// Beep instead of playing sound in Linux, at least until nsISound is +// implemented (bug ) and becomes asynchronous (bug 110385): +pref("accessibility.typeaheadfind.soundURL", "beep"); Comments in prefs files add to loading time, and provide little benefit. +// Beep instead of playing sound on Mac, because nsSound doesn't +// work on Mach-O and sounds crappy on carbon: +pref("accessibility.typeaheadfind.soundURL", "beep"); It's not that nsSound itself sound crappy on carbon, but that these .wav files sound crappy. They must have low bitrates or something. Mac's nsSound is capable of CD-quality sound (it just uses QuickTime, and can play MP3). Nuke the comment, anway. (/me, who can't help thinking that it's features like this that bloat mozilla)

Comment on attachment 106176 [details] [diff] [review] Same as last one, but set the mac default to "beep" Sorry for the delay; I finally managed to test the patch on a mach-o build (and got my system beep working so I could tell when it fired :-), and it does work for me. Seeking reviews.

Comment on attachment 106176 [details] [diff] [review] Same as last one, but set the mac default to "beep" r=aaronl

Can you address my points in comment 17?

Oh, sorry, Simon! I forgot that I hadn't made a patch yet addressing your comments yet. New patch coming (after my build finishes and I test it). I'm removing the comment for macprefs.js, but leaving the one in unix.js, because unix people do look at prefs files and comments there are very helpful. Besides, the file is already full of comments. If they're a performance penalty, should we perhaps strip them when we make the jar files? Should I file an RFE for that? > Why not just put the default sound url in the default prefs, and remove this > special "default" value? I did that in my first patch, and Aaron asked me in comment 5 to implement "default" as I've done it in the later patch.

>should we perhaps strip them when we make the jar files? all.js and friends are not part of the jar files... or were you not implying that, and just mentioning that it should happen at the same time?

Fixed. Happy beeping.

Comment on attachment 107504 [details] [diff] [review] Latest patch r=aaronl

thanks for fixing this, akkana. vrfy'd fixed on the following trunk builds: 2002.12.10.07 mach-o mozilla (OS X 10.2.2) - now get the system beep. 2002.12.09.08 comm on linux rh8 - now get the system beep. 2002.12.10.08 comm on win2k - now get the installed soundbyte.