Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: justin.lebar+bug, Unassigned)

Tracking

({dev-doc-complete})

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

I wasted an hour last night trying to figure out how to implement nsIClassInfo.  This should be much simpler to figure out.

It doesn't help that the MCD page [1] is out of date.  For instance, it references NS_DECL_CLASSINFO, which no longer exists.

I propose we remove all C++-specific instructions to comments in nsClassInfoImpl.h -- comments in the code have a much higher probability of being updated when the code changes.  (Also, speaking for myself, I'm much more likely to find a code comment than an MDC page.)  We can put a link to the comments in the wiki page.

I'll make an attempt at writing up this comment, but there's a lot about ClassInfo I don't understand.  I'll post some questions in a bit.

[1] https://developer.mozilla.org/en/Using_nsIClassInfo
cc'ing the usual suspects who can help review
Posted patch Patch v1 (obsolete) — Splinter Review
There are a few XXX's in the patch which I need help with.  bsmedberg, do you know the answers?
Attachment #533294 - Flags: feedback?(benjamin)
Comment on attachment 533294 [details] [diff] [review]
Patch v1

>diff --git a/xpcom/components/nsIClassInfo.idl b/xpcom/components/nsIClassInfo.idl

> /**
>- * Provides information about a specific implementation class
>+ * Provides information about a specific implementation class.  If you want
>+ * your class to implement nsIClassInfo, see nsIClassInfo.h for instructions --
>+ * you don't (XXX necessarily?) need to inherit from nsIClassInfo.

I'd be even stronger: implementation classes should not inherit from nsIClassInfo.

However, I'd put most of the docs in this file: the classinfo contract is a bit strange, in that we QI things to nsIClassInfo but expect to get a singleton, which breaks the normal XPCOM QI rules. That should be noted in this file, not just in nsClassInfoImpl.h. This is important when implementing nsIClassInfo in JavaScript.

>diff --git a/xpcom/glue/nsIClassInfoImpl.h b/xpcom/glue/nsIClassInfoImpl.h

>+ * XXX What about the NULL param, the get language helper callbacks?

I really don't know what this does, I think it's used for specialized DOM stuff. jst/mrbkap would know.
Attachment #533294 - Flags: feedback?(benjamin)
jst/mrbkap, thoughts on comment 4?
(In reply to comment #4)
> >+ * you don't (XXX necessarily?) need to inherit from nsIClassInfo.
> 
> I'd be even stronger: implementation classes should not inherit from
> nsIClassInfo.

I don't known that I'd be that strong :), there's nothing inherently wrong with implementing nsIClassInfo on a class itself, but it's often better to do it on a separate object since you can save yourself a vtable per object, but you do pay a slight performance penalty due to additional malloc overhead if you don't hand back a singleton.

> >+ * XXX What about the NULL param, the get language helper callbacks?
> 
> I really don't know what this does, I think it's used for specialized DOM
> stuff. jst/mrbkap would know.

Yup, this is all for getting from XPCOM object to a scriptable helper, i.e. nsIXPCScriptable in the case of JS (which is the only relevant language here, really). So unless you're doing some very specialized JS stuff, you can ignore this. The only user of this data that I know of is XPConnect.
(In reply to comment #6)
> there's nothing inherently wrong
> with implementing nsIClassInfo on a class itself,

Except that we do treat nsIClassInfo as a singleton, for example in XPConnect, and if nsIClassInfo is implemented in the component itself we often end up holding one instance of that component alive for the duration of the app. See also bug 658632. I'd make the comment say that the component shouldn't implement nsIClassInfo itself unless there is a very specific need for classinfo to be unique per instance. It's probably very rare, XTF does it currently but I'm not convinced that it really needs to.
Posted patch Patch v2Splinter Review
Attachment #533294 - Attachment is obsolete: true
Attachment #544825 - Flags: review?(peterv)
Comment on attachment 544825 [details] [diff] [review]
Patch v2

Review of attachment 544825 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsIClassInfoImpl.h
@@ +46,5 @@
> + * nsIClassInfo.  Implementing nsIClassInfo is particularly helpful if you have
> + * a C++ class which implements multiple interfaces and which you access from
> + * JavaScript.  If that class implements nsIClassInfo, the JavaScript code
> + * won't have to call QueryInterface on instances of the class; all methods
> + * from all interfaces will be available automagically.

from all interfaces returned from GetInterfaces
Attachment #544825 - Flags: review?(peterv) → review+
Thanks for the review, Peter!

Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/ff3401bfbf59
Whiteboard: [inbound]
Updated the wiki page, marking dev-doc-complete.
http://hg.mozilla.org/mozilla-central/rev/ff3401bfbf59
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.