1025395 - mach's webidl-example uses NS_IMPL_CYCLE_COLLECTION_INHERITED_0, which no longer exists

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

mach needs to be updated to use NS_ADDREF_IMPL_INHERITED, then list the attributes that should be collected. This will require figuring out types from the attributes in the webidl interface.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

The example generator doesn't output any data members.

It also doesn't output NS_ADDREF_IMPL_INHERITED_0 anywhere...

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Example WebIDL for MIDIOutput

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Example cpp output for webidl-examples run on MIDIOutput webidl

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I've attached the webidl and the corresponding cpp file put out by running mach webidl-example. There's a NS_ADDREF_IMPL_INHERITED_0 there.

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> There's a NS_ADDREF_IMPL_INHERITED_0 there.

There is?  Where?

I see NS_IMPL_ADDREF_INHERITED and I see NS_IMPL_CYCLE_COLLECTION_INHERITED_0.  Did you mean the latter?

If so, that never existed, as far as I know.  You're supposed to get a compile error and actually use the right thing there that CCs your members, or remove the CC bits if you have no members to cycle collect...  I'm definitely up for changes to the output to make that clearer.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Wow. Ok. I seem to have caught some sort of macro related dyslexia, sorry about that. They're like, all right next to each other. :| Will update bug.

Anyways, I meant NS_IMPL_CYCLE_COLLECTION_INHERITED_0. To fix it for the moment, I ended up just using NS_IMPL_CYCLE_COLLECTION_CLASS to initially get things to compile, but it sounds like the solution might be to have an error macro in there for when people try to just cut/paste like I did?

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Yeah, that might make sense.

Comment 8

[Axel Nennker]

The code that outputs NS_IMPL_CYCLE_COLLECTION_INHERITED_0 is here
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#13395

13391         if self.parentIface:
13392             ccImpl = dedent("""
13393 
13394                 // Only needed for refcounted objects.
13395                 NS_IMPL_CYCLE_COLLECTION_INHERITED_0(${nativeType}, ${parentType})
13396                 NS_IMPL_ADDREF_INHERITED(${nativeType}, ${parentType})
13397                 NS_IMPL_RELEASE_INHERITED(${nativeType}, ${parentType})
13398                 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(${nativeType})
13399                 NS_INTERFACE_MAP_END_INHERITING(${parentType})
13400 
13401                 """)

maybe that should be
NS_IMPL_CYCLE_COLLECTION_INHERITED(${nativeType}, ${parentTypes})
with parentTypes being the list of parent types?

Maybe this depends on whether the classes in the list are nsISupports or only the last one?
What does webidl-example output for the members of the list parentTypes?

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> maybe that should be

No, it shouldn't.  See comment 5.  Maybe we should put that into an actual comment in the generated code....

Comment 10

[Axel Nennker]

How about issuing something like:

#error if the class has refcounted members then add them variables NS_IMPL_CYCLE_COLLECTION_INHERITED(${nativeType}, ${parentType}, mMember1, mMember2) // nsCOMPtr<nsIMember1Class> mMember1; see WebIDL_bindings

instead of the current NS_IMPL_CYCLE_COLLECTION_INHERITED_0 code that yields a cryptic error message."... expected constructor, destructor, or type conversion before ‘MozExternalRefCountType’"

-Axel

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Or maybe we should actually have a NS_IMPL_CYCLE_COLLECTION_INHERITED_0?  I see some service worker code working around its lack with an mCCDummy member...

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Not doing reviews right now from comment #11)
> Or maybe we should actually have a NS_IMPL_CYCLE_COLLECTION_INHERITED_0?  I
> see some service worker code working around its lack with an mCCDummy
> member...

I think that can be eliminated. I have a patch.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> I think that can be eliminated. I have a patch.

I filed bug 1166488.

Anyways, the basic idea is that generally any time you might want to use INHERITED_0 you can instead just not declare any CC or ISUPPORTS stuff and inherit it all from your parent.  I suppose there could be some situation wherein you implement an additional nsIFoo interface, but don't have any additional CCed members, and then maybe you'd need it.  Also, you may want to override AddRef and Release to get better error messages, again without having any additional CCed members.  So I guess there are some use cases.

Comment 14

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Output a #error instead of a nonexistent macro for the CC bits of example codegen, to make it clearer to people using it what they should do with CC

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2579eae7696b

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2579eae7696b