Last Comment Bug 657917 - Update nsIClassInfo docs
: Update nsIClassInfo docs
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nobody; OK to take it and work on it
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-18 07:25 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-07-26 03:56 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.94 KB, patch)
2011-05-18 08:31 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (4.78 KB, patch)
2011-07-08 08:36 PDT, Justin Lebar (not reading bugmail)
peterv: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-05-18 07:25:35 PDT
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
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-05-18 07:40:00 PDT
cc'ing the usual suspects who can help review
Comment 2 Justin Lebar (not reading bugmail) 2011-05-18 08:31:45 PDT
Created attachment 533294 [details] [diff] [review]
Patch v1
Comment 3 Justin Lebar (not reading bugmail) 2011-05-18 08:33:50 PDT
There are a few XXX's in the patch which I need help with.  bsmedberg, do you know the answers?
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-06-07 08:13:47 PDT
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.
Comment 5 Justin Lebar (not reading bugmail) 2011-07-07 17:10:01 PDT
jst/mrbkap, thoughts on comment 4?
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-07 17:29:01 PDT
(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.
Comment 7 Peter Van der Beken [:peterv] 2011-07-08 01:30:51 PDT
(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.
Comment 8 Justin Lebar (not reading bugmail) 2011-07-08 08:36:50 PDT
Created attachment 544825 [details] [diff] [review]
Patch v2
Comment 9 Peter Van der Beken [:peterv] 2011-07-25 01:45:35 PDT
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
Comment 10 Justin Lebar (not reading bugmail) 2011-07-25 05:54:46 PDT
Thanks for the review, Peter!

Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/ff3401bfbf59
Comment 11 Justin Lebar (not reading bugmail) 2011-07-25 06:11:07 PDT
Updated the wiki page, marking dev-doc-complete.
Comment 12 Marco Bonardo [::mak] 2011-07-26 03:56:04 PDT
http://hg.mozilla.org/mozilla-central/rev/ff3401bfbf59

Note You need to log in before you can comment on or make changes to this bug.