Closed Bug 815454 Opened 7 years ago Closed 7 years ago

Clean up Traverse/Unlink of WebAudio

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch clean up (obsolete) — Splinter Review
- 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.
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 on attachment 685414 [details] [diff] [review]
clean up

Linux64 debug try run looks good.
https://tbpl.mozilla.org/?tree=Try&rev=9146c20240ef
Attachment #685414 - Flags: review?(bugs)
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
Attachment #685414 - Flags: review?(bugs) → review+
Thanks a lot for doing this, Andrew!  This is hot stuff!
> 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.
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.
Attached patch clean upSplinter Review
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.
Attachment #685414 - Attachment is obsolete: true
Attachment #685814 - Flags: review?(bugs)
Attachment #685814 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/795294e97485
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.