Created attachment 106171 [details] Patch

Created attachment 106180 [details] Patch

Comment on attachment 106180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106180&action=review > Source/WebCore/bindings/js/JSEventTarget.cpp:-182 > - return toJS(exec, globalObject, jsAudioNode); I notice you're removing this from the JSC code-path. Do you also need to make an appropriate change in CodeGeneratorJS.pm? I agree we should move over to Callback, but I don't want to land a change which will break web audio on any of the ports

(In reply to comment #3) > (From update of attachment 106180 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106180&action=review > > > Source/WebCore/bindings/js/JSEventTarget.cpp:-182 > > - return toJS(exec, globalObject, jsAudioNode); > > I notice you're removing this from the JSC code-path. Do you also need to make an appropriate change in CodeGeneratorJS.pm? You’re right; this could be problematic for JSC. > I agree we should move over to Callback, but I don't want to land a change which will break web audio on any of the ports Is JSC WebAudio in a buildable state? I may as well make onaudioprocess a callback. It would be nice if JavaScriptAudioNode had unit tests. Can you help me write one?

Two more follow-up questions: Do any other browsers have implementations/are working on implementations of Web Audio? Who is a good person to ask to review Web Audio patches?

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 106180 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=106180&action=review > > > > > Source/WebCore/bindings/js/JSEventTarget.cpp:-182 > > > - return toJS(exec, globalObject, jsAudioNode); > > > > I notice you're removing this from the JSC code-path. Do you also need to make an appropriate change in CodeGeneratorJS.pm? > > You’re right; this could be problematic for JSC. > > > I agree we should move over to Callback, but I don't want to land a change which will break web audio on any of the ports > > Is JSC WebAudio in a buildable state? I may as well make onaudioprocess a callback. Yes, it's possible to build for the mac port and the GTK project has an experimental version running, although their changes are not committed yet. I think it's best to have the complete change from EventListener -> Callback working before we land. I can help you test that this is working properly. You can also test this somewhat yourself by making sure that this page runs correctly both before and after the change: http://chromium.googlecode.com/svn/trunk/samples/audio/javascript-processing.html You can "view source" to see how this works -- pretty simple in this case (except for the WebGL part which isn't relevant to our test) > > It would be nice if JavaScriptAudioNode had unit tests. Can you help me write one? Yes, probably it would simply involve verifying that the callback is firing repeatedly.

(In reply to comment #5) > Two more follow-up questions: > > Do any other browsers have implementations/are working on implementations of Web Audio? > > Who is a good person to ask to review Web Audio patches? I'd like to be on the review list, and Kenneth Russell does many Web Audio reviews.

Should the callback take four parameters: void processAudio(JavaScriptAudioNode node, float playbackTime, sequence<AudioBuffer> inputBuffer, sequence<AudioBuffer> outputBuffer) Or should it take one object with these as properties? If it takes an object, what should the type of that object be named? (AudioProcessEvent isn’t a good name, because the object is no longer an event.)

(In reply to comment #8) > Should the callback take four parameters: > > void processAudio(JavaScriptAudioNode node, float playbackTime, sequence<AudioBuffer> inputBuffer, sequence<AudioBuffer> outputBuffer) > > Or should it take one object with these as properties? If it takes an object, what should the type of that object be named? (AudioProcessEvent isn’t a good name, because the object is no longer an event.) I don't want to change the existing API and break code that's already out in the field, so let's stick with one object with these as properties. How about re-naming like this "AudioProcessingEvent" -> "AudioProcessingCallbackInfo" ? Please also note that the current implementation has both "inputBuffer" and "outputBuffer" passed directly as type "AudioBuffer" instead of "sequence<AudioBuffer>" (as the spec currently describes). I'll change the spec to match the current behavior along with the new name we choose to replace AudioProcessingEvent.

So the proposed IDL is something like: interface JavaScriptAudioNode : AudioNode { attribute AudioProcessingCallback onaudioprocess; readonly attribute long bufferSize; } [Callback, NoInterfaceObject] interface AudioProcessingCallback { void processAudio(AudioProcessingCallbackInfo info); } [NoInterfaceObject] interface AudioProcessingCallbackInfo { attribute JavaScriptAudioNode node; readonly attribute float playbackTime; readonly attribute AudioBuffer inputBuffer; readonly attribute AudioBuffer outputBuffer; }

Dominic, yes this looks about right. I'm assuming that "processAudio()" is just internal IDL language for the C++ part and has no meaning in JavaScript? In other words, the actual JavaScript callback function can be any name. Sorry if I'm being stupid - just want to make sure. We can change "playbackTime" to be double instead of float -- see: https://bugs.webkit.org/show_bug.cgi?id=74030

(In reply to comment #11) > Dominic, yes this looks about right. I'm assuming that "processAudio()" is just internal IDL language for the C++ part and has no meaning in JavaScript? In other words, the actual JavaScript callback function can be any name. Sorry if I'm being stupid - just want to make sure. It depends. If you do this: var n = /* some JavaScriptAudioNode */ n.onaudioprocess = f; then f can be a function with any name. However if you do n = onaudioprocess = o; where o is an object that is not a function, then it will try to call o.processAudio(…). If that should be a different name, we can change it. > We can change "playbackTime" to be double instead of float -- see: > https://bugs.webkit.org/show_bug.cgi?id=74030 Got it. Thanks.

(In reply to comment #12) > (In reply to comment #11) > > Dominic, yes this looks about right. I'm assuming that "processAudio()" is just internal IDL language for the C++ part and has no meaning in JavaScript? In other words, the actual JavaScript callback function can be any name. Sorry if I'm being stupid - just want to make sure. > > It depends. If you do this: > > var n = /* some JavaScriptAudioNode */ > n.onaudioprocess = f; > > then f can be a function with any name. However if you do > > n = onaudioprocess = o; > > where o is an object that is not a function, then it will try to call o.processAudio(…). If that should be a different name, we can change it. Oh right, now I remember you explaining this to me. That sounds fine.

V8 is gone.