815454 - Clean up Traverse/Unlink of WebAudio

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: clean up

- Use the existing infrastructure for traversing nsTArray by defining ImplCycleCollectionTraverse for Input and Output.
- Collapse together AudioNode into WRAPPERCACHE_3 now that it can be done.
- Make use of NS_IMPL_CYCLE_COLLECTION_INHERITED_n
- PannerNode doesn't need to define its own cycle collector participant, as it has no additional fields to traverse or unlink, so it can just use AudioNode's participant.

Comment 2

[Andrew McCreight [:mccr8]]

This patch leaves AudioBuffer, with its weird array of JS pointers, as the only class in the webaudio/ directory that has to explicitly define its own Traverse and Unlink functions.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 685414 [details] [diff] [review]
clean up

Linux64 debug try run looks good.
https://tbpl.mozilla.org/?tree=Try&rev=9146c20240ef

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 685414 [details] [diff] [review]
clean up


>+inline void
>+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
>+                            mozilla::dom::AudioNode::Output& aField,
>+                            const char* aName,
>+                            unsigned aFlags)
>+{
>+  CycleCollectionNoteChild(aCallback, aField.mDestination.get(), aName, aFlags);
>+}
>+
>+inline void
>+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
>+                            mozilla::dom::AudioNode::Input& aField,
>+                            const char* aName,
>+                            unsigned aFlags)
>+{
>+  CycleCollectionNoteChild(aCallback, aField.mSource.get(), aName, aFlags);
>+}
>+

Do we need to have these in the .h file? I'd prefer keeping them in .cpp close to the
macro which implement the CC stuff.


>-NS_IMPL_CYCLE_COLLECTION_CLASS(PannerNode)
>-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(PannerNode, AudioNode)
>-NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(PannerNode, AudioNode)
>-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>-
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(PannerNode)
> NS_INTERFACE_MAP_END_INHERITING(AudioNode)
Do you need this, I mean MAP_BEGIN/END ?



Those addressed, r=me

Comment 5

[(no longer active)]

Thanks a lot for doing this, Andrew!  This is hot stuff!

Comment 6

[Andrew McCreight [:mccr8]]

> Do we need to have these in the .h file? I'd prefer keeping them in .cpp close to
> the macro which implement the CC stuff.

It depends. Ehsan, is it likely any other class will have fields with type AudioNode::Input or AudioNode::Output? If that's the case, then it should be in the header so those other classes can use it, but if this is just a private thing, then I can move it to the .cpp file.

> Do you need this, I mean MAP_BEGIN/END ?

Ah, right, I guess it will just inherit its QI in this case. Seems like I can remove it.

Comment 7

[Andrew McCreight [:mccr8]]

On IRC, Ehsan said Input/Output are probably just for AudioNode, so we can move those things into the .cpp files for now. If somebody else needs it, we can easily move it out.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: clean up

Could you just look at the PannerNode changes I made here to see if that's okay? I didn't bother with adding the CTOR/DTOR stuff, but I can add it if you think I should. We probably have this issue in a number of places already.

Comment 9

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/795294e97485

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/795294e97485