Implement AudioParamMap definitions

RESOLVED FIXED in mozilla62

Status

()

P2
normal
Rank:
15
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
mozilla62
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 disabled)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

59 bytes, text/x-review-board-request
karlt
: review+
qdot
: review+
Details
Comment hidden (empty)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Assignee: nobody → arnaud.bienner
(Assignee)

Comment 2

10 months ago
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.

Updated

10 months ago
Rank: 15
Priority: -- → P2
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?
Flags: needinfo?(continuation)
Attachment #8975329 - Flags: review?(karlt)
Attachment #8975329 - Flags: feedback+
Keywords: dev-doc-needed
(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.
Flags: needinfo?(continuation)
(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.
(Assignee)

Comment 7

10 months ago
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.
Flags: needinfo?(karlt)
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.
Flags: needinfo?(karlt)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

10 months ago
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 12

10 months ago
mozreview-review
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.)
Attachment #8975329 - Flags: review?(karlt) → review+
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.)
Attachment #8975329 - Flags: review?(continuation)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

10 months ago
Comment on attachment 8975329 [details]
Bug 1460907 - Implement AudioParamMap definitions.

Re-add review? for Andrew after updating the patch to fix Karl's comments.
Attachment #8975329 - Flags: review?(continuation)
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.
Attachment #8975329 - Flags: review?(continuation) → review?(kyle)
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.
Attachment #8975329 - Flags: review?(kyle) → review-
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.
(Assignee)

Comment 20

10 months ago
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 :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8981947 - Attachment is obsolete: true
Attachment #8981947 - Flags: review?(karlt)
(Assignee)

Comment 23

10 months ago
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.
Attachment #8975329 - Flags: review?(kyle)
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.
Attachment #8975329 - Flags: review?(kyle) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 26

10 months ago
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/
(Assignee)

Comment 28

10 months ago
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.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 29

10 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/426ab5b84824
Implement AudioParamMap definitions. r=karlt,qdot
Keywords: checkin-needed

Comment 30

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/426ab5b84824
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1473176
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”.
status-firefox62: fixed → disabled
You need to log in before you can comment on or make changes to this bug.