karlt: I've created a code review to have some feedback from you about this patch. I used TestInterfaceMapLike and MIDIInputMap as examples, but I'm really not sure what should be implemented here (maybe nothing more if the auto-generated code is enough). I believe adding some tests would help to know if we have everything we need, but I'm not sure what that would that look like. Maybe it will make more sense to have tests for the whole AudioWorklet implementation, which will also test AudioParamMap among other things.

Thank you, Arnaud. I'm learning here also. This looks a very similar use case to MIDIInputMap, and so I assume it makes sense to have a similar implementation, which this is. IIUC the generated code should be sufficient here. However, I'll ask some questions regarding how to choose from different implementation possibilities, because MIDIInputMap differs from AudioBuffer in ways that I didn't expect. We can probably depend on whole AudioWorklet tests to test this. For example, testing/web-platform/tests/webaudio/the-audio-api/the-audioworklet-interface/audioworklet-audioparam.https.html at least should test this to some extent (once the other pieces are in place).

Andrew, if the class doesn't need to inherit from nsISupports then should the _NATIVE_ CYCLE_COLLECTION methods be used, as in AudioBuffer? In bug 890543, AudioBuffer was changed to avoid inheriting from nsISupports. I don't see any reasons given beyond the lack of necessity to inherit. The benefit I guess would be that AddRef and Release don't need to be virtual. Is there guidance on when to inherit from nsISupports? Also, would it be better to store the nsPIDOMWindowInner in an nsWeakPtr, as in AudioBuffer, or is a strong reference usually used for parent windows?

(In reply to Karl Tomlinson (:karlt) from comment #4) > Andrew, if the class doesn't need to inherit from nsISupports then should the > _NATIVE_ CYCLE_COLLECTION methods be used, as in AudioBuffer? Yes. The one thing to keep in mind is that NATIVE doesn't support inheritance (because CC inheritance relies on QI), so if you have a subclass of this class that needs to add its own CCable members, you can't use NATIVE. > The benefit I guess would be that AddRef and Release don't need to be > virtual. Yes, that plus it is nice to avoid the nsISupports boilerplate. > Is there guidance on when to inherit from nsISupports? I think generally you should avoid it unless you actually need it. So here you probably don't need it to be nsISupports. > Also, would it be better to store the nsPIDOMWindowInner in an nsWeakPtr, as > in AudioBuffer, or is a strong reference usually used for parent windows? All of the parent pointers I've seen have been strong pointers. I'm a little surprised AudioBuffer uses a weak pointer, but I don't know exactly why it needs to be strong so maybe it makes sense there.

(In reply to Andrew McCreight [:mccr8] from comment #5) > > Also, would it be better to store the nsPIDOMWindowInner in an nsWeakPtr, as > > in AudioBuffer, or is a strong reference usually used for parent windows? > > All of the parent pointers I've seen have been strong pointers. I'm a little > surprised AudioBuffer uses a weak pointer, but I don't know exactly why it > needs to be strong so maybe it makes sense there. Thank you, Andrew. The common weak pointer case I've seen is DOMEventTargetHelper and derived classes that implement GetParentObject() with GetOwner(). The AudioBuffer situation was copied from this. History FWIW https://hg.mozilla.org/mozilla-central/rev/ce1ab99f8af891f71a7a4063eb3f5eececc570e1#l2.52 https://hg.mozilla.org/mozilla-central/rev/372ae5eae8b9041cf58783980ac91ad6fdbdbb91#l5.80 nsWeakPtr is certainly more available for nsISupportsWeakReference parents, but it sounds like whatever is simpler (presumably a strong pointer) is fine. In my mental model, adding more strong references makes cycles more complicated and more work to traverse, but I don't know that's a problem in practice. Weak references can have their own performance issues.

Karl: I don't know much about nsISupports and nsWrapperCache. Do you think we should avoid inheriting from them? Or one of them? What are the pros and cons here? I believe avoid inheriting things we don't really need is probably good, but on the other hand, code reuse is good too. If we don't reimplement one of those classes, I'm not sure what should be reimplemented.

nsISupports is pretty much a model used to support inheritance and safe dynamic casts in C++ objects. nsISupports also happens to provide declarations of AddRef() and Release(). Sometimes a class needs to inherit from nsISupports because it is passed to some Gecko methods that expects an nsISupports object. We don't expect to need to define any other C++ classes inheriting from AudioParamMap and we don't expect AudioParamMap to need to be an nsISupports for any other reason. We can therefore define AudioParamMap in a way that does not inherit from nsISupports, which should mean less code, and code which is slightly more cpu-efficient. The NS_INTERFACE_MAP_* macros should no longer be required as they provide the dynamic casts. AudioBuffer is a class that supports webidl bindings and does not inherit from nsISupports. That makes it a useful model to copy in one sense, but it is more complicated than necessary for AudioParamMap because AudioBuffer::mJSChannels needs to be traced for garbage collection. AudioParamMap does not need to bother with that and so should be fine to stick with the simple NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AudioParamMap, mParent). (Also keep nsCOMPtr<nsPIDOMWindowInner> mParent.) The changes to make to AudioParamMap should mirror those in https://hg.mozilla.org/mozilla-central/rev/33937005b8e4872e2f72807cbe0b934ce815b92e I guess the webidl bindings generated assume inheritance from nsWrapperCache to support accessing the class from JS. AudioParamMap should therefore inherit from nsWrapperCache. Code reuse is good, but I don't think AudioParamMap benefits much from nsISupports. The difference may be minor, but this is such a common pattern that it is worth getting it right, because it will likely be copied around.

Thanks Karl: I've updated the patch. The interdiff of webidl/moz.build looks wrong, but I think this might be because I rebased my work between the two changes. The whole diff is OK.

(In reply to Arnaud Bienner from comment #10) > Thanks Karl: I've updated the patch. > > The interdiff of webidl/moz.build looks wrong, but I think this might be > because I rebased my work between the two changes. The whole diff is OK. Yes, mozreview's associated-change detection in interdiff has been disabled to work around a bug that sometimes hid real differences. Usually it's best to push updated changesets to mozreview on the same base revision as the original. Sometimes code has changed enough that a rebase is necessary or worthwhile, in which case pushing changesets with only the rebase separately from pushing real changes to the patch provides some useful interdiffs. None of that is a problem here though as the patches are not so complex. Putting the following in ~/.hgrc avoids the need to answer a question after every push. Changes can always be checked on reviewboard before submission. [reviewboard] autopublish = False

Comment on attachment 8975329 [details] Bug 1460907 - Implement AudioParamMap definitions. https://reviewboard.mozilla.org/r/243642/#review253400 Looks good, thanks. We need approval from a DOM peer because a webidl file changed. ::: dom/media/webaudio/AudioParamMap.h:26 (Diff revision 2) > + nsPIDOMWindowInner* > + GetParentObject() const Keep the type and function name on the same line where practical for declarations, as for WrapObject() below, but even for inline definitions: nsPIDOMWindowInner* GetParentObject() const ( { return mParent; } also on the same line is optional.)

Comment on attachment 8975329 [details] Bug 1460907 - Implement AudioParamMap definitions. (No value filing a mozreview bug on the failure to transfer the review request, because it won't get fixed.)

Comment on attachment 8975329 [details] Bug 1460907 - Implement AudioParamMap definitions. Re-add review? for Andrew after updating the patch to fix Karl's comments.

Comment on attachment 8975329 [details] Bug 1460907 - Implement AudioParamMap definitions. I noticed that the mode lines on the files you added are wrong. Please fix those. See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line I'm not very familiar with WebIDL. Maybe qdot could review the WebIDL change you are making.

Comment on attachment 8975329 [details] Bug 1460907 - Implement AudioParamMap definitions. https://reviewboard.mozilla.org/r/243642/#review253752 Code looks fine, but has this been on a run through try yet? This looks like it's missing an entry in dom/tests/mochitest/general/test_interfaces.js, and since this is a public exposed interface (when the pref is flipped on), that's going to fail.

I don't see any such failures at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7793cc6b6444e756ead3ddb09939b3d47d011a43 but I don't know why test_interfaces would fail before the pref is turned on. Arnaud, I assume Kyle is wanting for an entry such as https://hg.mozilla.org/mozilla-central/rev/21fc5c9ae628#l18.16

Ah ok, didn't know when this would be pref'd on or if worklet was already live on nightly or something. Would still be nice to get it done alongside the interface landing, since it is expected to be exposed at some point. Filing it as disabled should be fine for now.

Actually I added a test to test_interfaces, while removing the pref, and tested that locally. But I wasn't sure how to keep this with the pref being turned off, so it wasn't part of my patch. Your suggestion (added as disabled) looks good, I'll add that :)

Sorry for messing up the review, and accidentally created a new commit (and so created a new review). Should be OK now. I updated test_interfaces.js as discussed, and fix mode line.

Comment on attachment 8975329 [details] Bug 1460907 - Implement AudioParamMap definitions. https://reviewboard.mozilla.org/r/243642/#review254140 ::: dom/media/webaudio/AudioParamMap.cpp:7 (Diff revision 4) > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ > + > +#include <AudioParamMap.h> nit: Just noticed this, we usually use quotes instead of <> for headers to prioritize what's in the current directory. I'm not aware of any system headers named AudioParamMap.h, but quotes would keep things consistent.

Thanks for the review. I've updated the patch: it should be ready to go. Karl: do you think this should go through try server again? Also, how to get the patch landed once it gets r+? I remember about adding "checkin-needed" keyword but I'm not sure if this is still the current procedure.

A try run is typically required for checkin-needed. Given a new test has been added, I've triggered another try run to be sure. I can vouch if you'd like to follow the procedure for Level 1 commit access to allow you to push to try: https://www.mozilla.org/en-US/about/governance/policies/commit/

I already had level 1 commit, but since I haven't contributed in a while, I'm not sure if it is still active. I'll try for the next bug/patch ;) Looks like the try build passed, so I will request checkin.

Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/426ab5b84824 Implement AudioParamMap definitions. r=karlt,qdot

Documentation team note: AudioWorklet is currently not expected to ship enabled by default until sometime early in 2019. Given that and some things which are still in flux, we are holding off on documenting it until it’s closer to ready. See bug 1473176, “Document AudioWorklet”.