97667 - nsIInterfaceRequestor.idl needs freezing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Judson Valeski]

components using XPCOM interract w/ interfaces that use nsIInterfaceRequestor.
This interface needs freezing. Currently the .idl wedges in nsCOMPtr which we
haven't frozen yet, but will obviously need to.

Comment 1

[Judson Valeski]

seeking review from dougt, and sr from scc.

be aware, if this goes in and nsIInterfaceRequestor needs to change, we're onto
nsIInterfaceRequestor2

Index: nsIInterfaceRequestor.idl
===================================================================
RCS file: /cvsroot/mozilla/xpcom/base/nsIInterfaceRequestor.idl,v
retrieving revision 1.5
diff -u -r1.5 nsIInterfaceRequestor.idl
--- nsIInterfaceRequestor.idl   1999/11/29 20:40:38     1.5
+++ nsIInterfaceRequestor.idl   2001/08/30 21:25:56
@@ -32,6 +32,8 @@
  * you must be able to QI on B to get back to A.  This interface however
  * allows you to obtain an interface C from A that may or most likely will not
  * have the ability to get back to A.
+ *
+ * @status FROZEN
  */

 [scriptable, uuid(033A1470-8B2A-11d3-AF88-00A024FFC08C)]

Comment 2

[Doug Turner (:dougt)]

Does this apply to the exported helper function:

do_GetInterface( nsISupports* aSource, nsresult* error = 0 )

and related nsGetInterface class.?  How do we deal with its base class
nsCOMPtr_helper?

Comment 3

[Judson Valeski]

technically it does not, but that would suggest those be removed from this file
and into another; do we want to move things around, or call those frozen too?

Comment 4

[Doug Turner (:dougt)]

can't really call them froze if the nsCOMPtr_helper isn't, right?

Comment 5

[Judson Valeski]

right, we'd have to drag that in too. we can either

- say that "FROZEN" applies *only* to the interface (this would go against
current standard convention).
- say it applies to everything in the file (current convention).


If the latter, we need to drag in the nest and FREEZE it all which is generally
a long, nasty process. I'm asking if we should do that here.

- do_GetInterface() - easy enough to freeze IMO
- nsGetInterface - I think this recently snuck in and am bumbed about it. do we
want to freeze it (and it's parent class), or move it into another file?
- nsCOMPtr - we're going to freeze it eventually, is this the impetus for that?

Comment 6

[Doug Turner (:dougt)]

scott, how close is nsCOMPtr to being api frozen?

Comment 7

[Judson Valeski]

new tree-wide patch forthcoming, I ran w/ a direction dbaron raised. Namely,
drawing out the stuff we're not necessarily settled on, out into
nsIInterfaceRequestorUtils.h.

Comment 8

[Judson Valeski]

Attachment: tree-wide mods to separate util stuff from nsIInterfaceRequestor.idl into it's own .h file.

Comment 9

[Judson Valeski]

Attachment: new nsIInterfaceRequestorUtils.h file.

Comment 10

[Judson Valeski]

what do you guys think?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems fine to me, except you have some extraneous makefile changes in
netwerk/core/public (I don't even have that directory) and both headers are
included twice in nsDirectoryViewer.cpp.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

s/have/remember having/

of course I do have it...

Comment 13

[Judson Valeski]

hmm. the makefile mods are for something else, and will be checked in
independently. I'm removing dead wood in there, separate bug, removed from tree.

thanks for catching the double include... removed.

new patch forthcoming.

Comment 14

[Judson Valeski]

Attachment: seeking r=dbaron, and sr=dougt. new .h will be added to xpcom mac prj for export as part of the checkin.

Comment 15

[Judson Valeski]

there are a few #includes in the commercial tree which will be addressed as well
for the checkin.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 47972 [details] [diff] [review]
seeking r=dbaron, and sr=dougt. new .h will be added to xpcom mac prj for export as part of the checkin.

r=dbaron if you add to the EXPORTS line in Makefile.in as well :-)

Comment 17

[Doug Turner (:dougt)]

looks good to me.  Maybe you should just include nsIInterfaceRequestorUtils.h?

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 47972 [details] [diff] [review]
seeking r=dbaron, and sr=dougt. new .h will be added to xpcom mac prj for export as part of the checkin.

sr=me.  Maybe you should just include "nsIInterfaceRequestorUtils.h"
 instead of both headers?

Comment 19

[Judson Valeski]

Attachment: tree-wide added new .h to the EXPORTS macro.

Comment 20

[Judson Valeski]

Comment on attachment 48183 [details] [diff] [review]
tree-wide added new .h to the EXPORTS macro.

carrying over r=dbaron, and sr=dougt from previous patch.

Comment 21

[Judson Valeski]

doug and I chatted off-line re: the header separation/combination. we're
sticking w/ what I have here... explicity separation between interface and utils
(#include both).

Comment 22

[Judson Valeski]

Attachment: new patch for nsIInterfaceRequestor.idl. comments have been updated. seeking r=dbaron, sr=dougt

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 48221 [details] [diff] [review]
new patch for nsIInterfaceRequestor.idl. comments have been updated. seeking r=dbaron, sr=dougt

has sr if r=dbaron

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 48221 [details] [diff] [review]
new patch for nsIInterfaceRequestor.idl. comments have been updated. seeking r=dbaron, sr=dougt

There are tabs on these two lines:
>+	/**
>+	 * Retrieves the specified interface pointer.


>+    * @param result [out] The interface pointer to be filled out if
>+    *               the interface is accessible.

Perhaps "filled in" is preferable to "filled out"?


>+    * @return NS_OK - interface was successfully returned.
>+    *         NS_NOINTERFACE - interface wasn't accessible.
>+    *         NS_ERROR_FAILURE - method failure.

Do you really want to limit the return values like this?  I
could imagine returning NS_ERROR_OUT_OF_MEMORY, or perhaps any
other error propagated from a failure of an |Init| method.  And
perhaps the description of NS_NOINTERFACE should be "interface
not accessible", since I would think NS_NOINTERFACE should mean
that the object does not, in general, support |getInterface| to
that interface.


>+    * @status FROZEN

Should there also be some indication that the interface is
frozen?  (I don't know the conventions, and you do, but it
just seems odd.)


r=dbaron (although if scc is around, it might be good to ping
him as well)

Comment 25

[Judson Valeski]

Attachment: latest nsIInterfaceRequestor.idl mods. taking dbaron's suggestions/fixes.

Comment 26

[Judson Valeski]

I've addressed dbaron's concerns:

- tabs removed.
- @status FROZEN moved from the method to the interface comments above it.
- "filled in" instead of "filled out"
- weazeled out of the confining ERROR return vals by saying it can return NS_ERROR*

Comment 27

[Judson Valeski]

Comment on attachment 48300 [details] [diff] [review]
latest nsIInterfaceRequestor.idl mods. taking dbaron's suggestions/fixes.

carrying over r/sr

Comment 28

[Judson Valeski]

checked in.