Comment on attachment 662767 [details] [diff] [review] Patch (v1) >+++ b/content/media/webaudio/AudioBufferSourceNode.h >+#pragma once Huh, nifty. I guess at this point we can be pretty sure all our compilers support this? >+++ b/content/media/webaudio/AudioNode.cpp >+AudioNode::WrapObject(JSContext* aCx, JSObject* aScope, Since there are never objects that have AudioNode.prototype as their proto, I think we should nix this method altogether (since it will never be called; only subclass overrides will be called). Further, we should add 'concrete': False to the descriptor for this interface in Bindings.conf. That will skip generating various code that's not needed if there are no objects that directly implement this interface (and in particular, will actually skip generating a AudioNodeBinding::Wrap method). >+++ b/content/media/webaudio/AudioSourceNode.cpp If this also can't be directly implemented, then we should mark this as 'concrete':False as well. Please check that? >+++ b/dom/bindings/Bindings.conf >+# * prefable - Indicates whether this bindings should be disabled if the >+# global pref for Web IDL bindings is set to false. This is a >+# risk mitigation strategy and it will cause all of the Web IDL >+# bindings to fall back "all of the Web IDL bindings marked as prefable" >+'AudioNode' : { >+ 'nativeType': 'AudioNode', You don't need the nativeType if the C++ class is actually mozilla::dom::InterfaceName, as it is for all of your things. Yes, that means that some of your entries here will look like this: 'AudioBufferSourceNode': { }, which is dumb, and I though I had a bug somewhere about making it possible to just skip listing a descriptor altogether in that situation.... >+++ b/dom/webidl/AudioBufferSourceNode.webidl >+ const unsigned short UNSCHEDULED_STATE = 0; More of a spec comment... Should this be a WebIDL enum instead? Or is the intent to allow order comparisons between .playbackState and these states? Or is it just "spec what people implemented" at this point? If you'd prefer, I can raise those questions on the spec myself. >+++ b/dom/webidl/AudioNode.webidl >+ readonly attribute mozAudioContext context; I'd prefer it if this were: readonly attribute AudioContext context; as in the spec. And to make that work, just throw a: typedef mozAudioContext AudioContext; in AudioContext.webidl. That way when we unprefix we only have to change that one file. >+++ b/dom/webidl/WebIDL.mk > AudioContext.webidl \ >+ AudioBufferSourceNode.webidl \ Those two should be in the opposite order. r=me with those nits.

(In reply to Boris Zbarsky (:bz) from comment #2) > Comment on attachment 662767 [details] [diff] [review] > Patch (v1) > > >+++ b/content/media/webaudio/AudioBufferSourceNode.h > >+#pragma once > > Huh, nifty. I guess at this point we can be pretty sure all our compilers > support this? It works on all compilers that I've heard of! > >+++ b/content/media/webaudio/AudioNode.cpp > >+AudioNode::WrapObject(JSContext* aCx, JSObject* aScope, > > Since there are never objects that have AudioNode.prototype as their proto, > I think we should nix this method altogether (since it will never be called; > only subclass overrides will be called). Makes sense. > Further, we should add > > 'concrete': False > > to the descriptor for this interface in Bindings.conf. That will skip > generating various code that's not needed if there are no objects that > directly implement this interface (and in particular, will actually skip > generating a AudioNodeBinding::Wrap method). Done. Will also do the same for AudioSourceNode. > >+'AudioNode' : { > >+ 'nativeType': 'AudioNode', > > You don't need the nativeType if the C++ class is actually > mozilla::dom::InterfaceName, as it is for all of your things. Well I had to add the headers to EXPORTS_mozilla/dom, and once I did that, it worked perfectly! > Yes, that > means that some of your entries here will look like this: > > 'AudioBufferSourceNode': { > }, > > which is dumb, and I though I had a bug somewhere about making it possible > to just skip listing a descriptor altogether in that situation.... I filed bug 792980 for that! > >+++ b/dom/webidl/AudioBufferSourceNode.webidl > >+ const unsigned short UNSCHEDULED_STATE = 0; > > More of a spec comment... Should this be a WebIDL enum instead? Or is the > intent to allow order comparisons between .playbackState and these states? Aren't Web IDL enums just over strings? Can we have numeric enums too? > Or is it just "spec what people implemented" at this point? If you'd > prefer, I can raise those questions on the spec myself. No, I can raise spec issues myself, and have already fixed a bunch of things to make it Web IDL compatible. But yeah it's mostly a document of what WebKit has done, but it's getting better! > >+++ b/dom/webidl/AudioNode.webidl > >+ readonly attribute mozAudioContext context; > > I'd prefer it if this were: > > readonly attribute AudioContext context; > > as in the spec. And to make that work, just throw a: > > typedef mozAudioContext AudioContext; > > in AudioContext.webidl. That way when we unprefix we only have to change > that one file. Good idea!

... and backed out because of a leak which seems pretty plausible to be my fault: https://hg.mozilla.org/integration/mozilla-inbound/rev/904a52466167 https://tbpl.mozilla.org/php/getParsedLog.php?id=15392857&tree=Mozilla-Inbound&full=1

> Aren't Web IDL enums just over strings? Yes. The question is whether there's actually a good reason for the numeric values here or whether string values (which are more self-explanatory) would do better. Thanks for filing the followup bug on empty descriptor stuff!

As far as the leak goes, AudioContext needs to tell CC about its ref to mDestination, I think... Sorry I missed that. :(

(In reply to Boris Zbarsky (:bz) from comment #7) > As far as the leak goes, AudioContext needs to tell CC about its ref to > mDestination, I think... Sorry I missed that. :( Indeed! Fixed and relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/63d670f5bde1 (In reply to Boris Zbarsky (:bz) from comment #6) > > Aren't Web IDL enums just over strings? > > Yes. The question is whether there's actually a good reason for the numeric > values here or whether string values (which are more self-explanatory) would > do better. Hmm, I see. I'll bring that up on the spec side.

Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.

Many tests have been added in other bugs.