syd was mean to you guys, BeOS is very sound capable, please undo his damage.

minor note: nsSound is currently being misused as a non service and will probably be converted to be a service. hopefully beos won't care :).

something like that, but I didn't notice that either Play() or PlaySystemSound() was ever called (used uncommented printfs). Only nsSound::Beep() was called once for me. So couldn't test till now. Also I suspect that i should convert nsURL using GetNativePath, and i'm unsure if autostring in PlaySystemSound() provides real full path.

It works. Managed to test nsSound::Play() from Preferences->Privacy & Security->PopupWindows by selecting path to some sound and pushing Preview button.

removed mSound from Sound.h, fixed InitCheck()

oops, StreamerLoader is probably non-needed with current implementation in nSound.h. Anyway, waiting for timeless comments

Comment on attachment 200414 [details] [diff] [review] patch diff -up6 will try to use Init()

hope final for now

I was planning to look at that far into future :) Good to see it worked on.

Can one nsSound object play in polyphony? Otherwise this looks ok to me. If destructor don't need to stop sound and such.

No, single object is intended to play only one sound. BSimpleGameSound is rather initializer for GameSound instance with just that sound. You need different BSimpleSound instances to play different sounds. So, for example in game with standard set of sounds, you'd (pre)create set of BSimpleGameSound-s - e.g. KickSound, JumpSound etc and use those for different events. But if you mean if it is capable to be start playing twice or more (almost) simultaneously, e.g. from different threads, i don't know, if to be honest:(

Comment on attachment 200427 [details] [diff] [review] patch review request. Though, patch may be reconsidered in future to be able to play sounds from web, if this class is supposed to do it.

why does the patch work? BSimpleGameSound is documented to take filenames, not URLs. Also, why don't you need to unescape the spec?

Re:biesi It works some way. As you see above Probably that way i get from URL/URI normal local path acceptable for BSimpleSoundPlayer. But if you know good method to test just ::Play (not ::PlaySystemSound) and advise me about that URL->local_file_path magic, i wil be glad to fix it.

Comment on attachment 200427 [details] [diff] [review] patch It seems you are right biesi. current code in ::Play() prefixes path with "http://", so any advise is welcome

I'd use a streamloader and pass the raw data to BSimpleGameSound

or actually, nsIDownloader is probably better.

Universal version with StreamLoader

Comment on attachment 200533 [details] [diff] [review] patch please use nsIDownloader if you want a temp file

OK, so if you want to change to gtk-style some day, I suppose that's ok. I don't think this is particularly great though. but please at least delete the file at some point (e.g. in the destructor)

That's not gtk-only approach. Se let it be at least in part of code similar to other platforms. It makes maintenance easier. Removed all extra debug crap where you suspected wrong identation, added instant unlink, moved Init() from ::Play() to place before real BSimpleGameSound init.

same as previous. one space added, two spaces replaced by tab, one {} pair removed

Comment on attachment 200544 [details] [diff] [review] patch + printf("Filed load sound file"); do you mean "Failed to load sound file"? ;) + nsresult rv = NS_ERROR_FAILURE; I'd move it to the line where it is first used: + nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(soundTmp)); + // also we can play it from data, but it needs parsing format and "we can also play it from data" + static const char *kSoundTmpFileName = "mozsound"; I believe it's more efficient to use: static const char kSoundTmpFileName[] = "mozsound"; hm, although that may have been ELF only... + //printf("Playing sound file%s\n",soundFilename.get()); I'm not a fan of commented out debug lines... can you remove it? (or put it into #ifdef DEBUG_fyysik, whatever) + fwrite(data, dataLen, 1, fp); well, strictly speaking you should exchange dataLen and 1 here... looks like lack of disk space won't be handled so well here + fflush(fp); no need to do that, fclose will do that implicitly. + mSound = new BSimpleGameSound(soundFilename.get()); hm... why not do that in Init(), which is kinda designed for this kind of thing? + return rv; this returns NS_OK if mSound was null or InitCheck() returned failure, that seems wrong... + printf("Play Beep\n"); should probably remove this printf... + NS_ENSURE_SUCCESS(rv,rv); spaces after commas, please (many times) r=biesi

"+ mSound = new BSimpleGameSound(soundFilename.get()); hm... why not do that in Init(), which is kinda designed for this kind of thing?" How? Where is parameter in Init()? Through module-wide variable? Don't look as elegant solution. Also as this isn't private method, we cannot be sure that it wouldn't be called at not so good time/place. BSimpleGameSound(sound) is very special object and can be intialized with sound paramter only once, and only in constructor. If it has itself Init()-like method, it might be reasonble. "+fflush(fp); no need to do that, fclose will do that implicitly." Yeah, i know. But i had several times strange experience with that in BeOS. So prefer to be absolutely sure. What may be really extra here, actually - setting no-looping flag for sound, but again, i wish to be absolutely sure also here. Other points taken.

> How? Where is parameter in Init()? yeah, sorry, I missed that.

checked in trunk with biesi's corrections Checking in mozilla/widget/src/beos/nsSound.h; /cvsroot/mozilla/widget/src/beos/nsSound.h,v <-- nsSound.h new revision: 1.8; previous revision: 1.7 done Checking in mozilla/widget/src/beos/nsSound.cpp; /cvsroot/mozilla/widget/src/beos/nsSound.cpp,v <-- nsSound.cpp new revision: 1.21; previous revision: 1.20 done

fixed mistypo /cvsroot/mozilla/widget/src/beos/nsSound.cpp,v <-- nsSound.cpp new revision: 1.22; previous revision: 1.21 and checked in Makefile.in Checking in mozilla/widget/src/beos/Makefile.in; /cvsroot/mozilla/widget/src/beos/Makefile.in,v <-- Makefile.in new revision: 1.49; previous revision: 1.48

Blocks Firefox 2.0 tracker.

Doug, could you check this out? I don't have a working soundcard and I needed to make a modification to work around an API change. If you say it works I'll ask approval for the change.

Patch applies cleanly but does not build - see attachment. IIR, there was an additional "include" statement needed somewhere, but I don't remember the details. fyysik might, or I can start digging.

probably that's -lgame in Makefile.in which is missing. Don't remember in which bug and when I really added it

(In reply to comment #30) > probably that's -lgame in Makefile.in which is missing. > Don't remember in which bug and when I really added it > -lgame needs to be added to mozilla/browser/app/Makefile.in on branch, in BeOS libs section, line 148. Same line as -ltracker. It's already in mozilla/widget/src/beos/Makefile.in, so it took me a while to figure out where else it is needed. Tested by opening "edit/find in this page" and typing junk. It works with this change.

should probably also remove "printf("Play Beep\n");" from line 159 of patch for 1.8 branch.

Removed dprintf. Doug, please test in combination with bug 314518 in a static build. If it works and builds, it is ready to go in.

(In reply to comment #33) > Doug, please test in combination with bug 314518 in a static build. If it works > and builds, it is ready to go in. > Applied this patch plus 314518. Patches apply cleanly, sound now works OK (tested by searching for non-existent text in Edit/Find in this page)

Comment on attachment 231383 [details] [diff] [review] Patch for 1_8 branch (2) Requesting approval for landing in the MOZILLA_1_8_BRANCH. This is a BeOS-only change and does not affect other platforms.

Comment on attachment 231383 [details] [diff] [review] Patch for 1_8 branch (2) a=schrep for drivers for BeOS only change.

Checking in mozilla/widget/src/beos/Makefile.in; /cvsroot/mozilla/widget/src/beos/Makefile.in,v <-- Makefile.in new revision: 1.47.14.3; previous revision: 1.47.14.2 done Checking in mozilla/widget/src/beos/nsSound.cpp; /cvsroot/mozilla/widget/src/beos/nsSound.cpp,v <-- nsSound.cpp new revision: 1.19.28.1; previous revision: 1.19 done Checking in mozilla/widget/src/beos/nsSound.h; /cvsroot/mozilla/widget/src/beos/nsSound.h,v <-- nsSound.h new revision: 1.7.28.1; previous revision: 1.7 done

This bug is fixed in the branch and the trunk, closing.