|NS_DEFINE_IID| is bad; use |NS_GET_IID| instead

NEW

Status

()

Core
XPCOM
P3
normal
18 years ago
8 years ago

People

(Reporter: Scott Collins, Assigned: jag (Peter Annema))

Tracking

Trunk
Future
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

18 years ago
|NS_DEFINE_IID| is the old space-wasting scheme.  |NS_GET_IID| is the (not so) 
new, accepted, more efficient (on most platforms) scheme.  We need to root out 
all the old instances of |NS_DEFINE_IID| and replace the uses of their defined 
IIDs with |NS_GET_IID(type)|.
(Reporter)

Comment 1

18 years ago
We've already agreed this is a P1; see
  <http://www.mozilla.org/projects/xpcom/XPCOM_plans.html>

There are thousands of occurances.  I don't think we can get them all by M14; but 
I'll have to do work on them in M14 to get them done by M15.  Probably need to 
file individual bugs on different sections of the code to get this done.
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: M15

Updated

18 years ago
Blocks: 21326
(Reporter)

Comment 2

18 years ago
Another one of those sweeping changes that fits in between everything else.  
There are still more instances than lxr is willing to display.
Target Milestone: M15 → M20
(Reporter)

Comment 3

17 years ago
mass re-assigning to my new bugzilla account
Assignee: scc → scc
Status: ASSIGNED → NEW

Updated

17 years ago
Status: NEW → ASSIGNED

Comment 4

17 years ago
dp is no longer @netscape.com. changing qa contact to default for this product
QA Contact: dp → kandrot
I've been hoping that we could move even further and try to minimize the number
of places where we need to use NS_GET_IID as well.  NS_GET_IID forces the
programmer to specify the type of a variable in two different places and does
not check that the types are the same -- it is therefore not typesafe, and I
think we should prefer the typesafe functions available that will reduce the
risk of serious errors:  the nsCOMPtr helpers (do_QueryInterface,
do_CreateInstance, do_GetService, do_QueryReferent) and the function templates
for out parameters (CallQueryInterface, CallCreateInstance, CallGetService,
CallQueryReferent).  Many of the other uses of NS_GET_IID are in hand-written
QueryInterface methods that would be better written using the NS_IMPL_ISUPPORTSn
macros.
(Assignee)

Comment 6

14 years ago
Created attachment 128085 [details] [diff] [review]
Get rid of a bunch of unused NS_DEFINE_IID, NS_DEFINE_CID

I grep'ed my tree for NS_DEFINE_.ID, parsed the output so I had a list of
(filename, cidVariableName), then did a grep -c to get the count of the
variable name in that file. If the count is 1, that means the file only has the
NS_DEFINE_.ID in it, and it can be stripped (unless it's a header, had to do
some manual checking there and undo a few removals). Initially I manually
removed these NS_DEFINE_.IDs and removed a few unneeded #includes in the
process, but that took forever so I wrote a little script to remove it for me
(hence no further #include clean-up).
(Assignee)

Updated

14 years ago
Attachment #128085 - Flags: superreview?(dbaron)
Attachment #128085 - Flags: review?(caillon)
Attachment #128085 - Flags: superreview?(dbaron) → superreview+
jag: great work.

Do we need to deprecate NS_DEFINE_IID harder, or are all the occurrences old?

/be
(Assignee)

Comment 8

14 years ago
I don't know, I could find out. It doesn't seem too hard to completely get rid
of NS_DEFINE_IID, I might go play with that this week.
Comment on attachment 128085 [details] [diff] [review]
Get rid of a bunch of unused NS_DEFINE_IID, NS_DEFINE_CID

r=caillon
Attachment #128085 - Flags: review?(caillon) → review+

Comment 10

14 years ago
*** Bug 61624 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Assignee: scc → jag
Status: ASSIGNED → NEW

Updated

11 years ago
QA Contact: kandrot → jag
Assignee: jag → nobody
QA Contact: jag → xpcom
(Assignee)

Comment 11

10 years ago
I have a patch locally that leaves me with this:

ipc/ipcd/extensions/dconnect/src/ipcDConnectService.cpp:static NS_DEFINE_IID(kDConnectStubID, DCONNECT_STUB_ID);
layout/mathml/base/src/nsIMathMLFrame.h:static NS_DEFINE_IID(kIMathMLFrameIID, NS_IMATHMLFRAME_IID);
layout/tables/nsTableRowGroupFrame.cpp:  static NS_DEFINE_IID(kITableRowGroupIID, NS_ITABLEROWGROUPFRAME_IID);
xpcom/proxy/src/nsProxyEvent.cpp:NS_DEFINE_IID(kFilterIID, NS_PROXYEVENT_FILTER_IID);
xpcom/proxy/src/nsProxyEventObject.cpp:NS_DEFINE_IID(kFilterIID, NS_PROXYEVENT_FILTER_IID);
xpcom/reflect/xptcall/tests/eVC4/TestXPTCInvokeInIDE.cpp:static NS_DEFINE_IID(kTheCID, INVOKETESTTARGETINTERFACE_IID);

I've removed them if they were unused, replaced QueryInterface implementations with NS_IMPL_ISUPPORTSx, replaced QueryInterface calls with {do_,Call}QueryInterface, replaced NS_DEFINE_IID() for CIDs with NS_DEFINE_CID(), and replaced the rest with NS_GET_IID()

Oh, and 'coz I couldn't help myself I've thrown in some random clean-up. I don't think there's too much, but I'll remove any you'd rather see in a different bug.

Once my tree finishes building I'll attach a patch.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Assignee: nobody → jag
Status: ASSIGNED → NEW
(Assignee)

Comment 12

10 years ago
Created attachment 276974 [details] [diff] [review]
Backup. This compiles on Mac.

Comment 13

10 years ago
Comment on attachment 276974 [details] [diff] [review]
Backup. This compiles on Mac.

ns4xPlugin.cpp(878) : error C2065: 'kCPluginManagerCID' : undeclared identifier
ns4xPlugin.cpp(2284) : error C3861: 'kCPluginManagerCID': identifier not found, even with argument-dependent lookup
make[5]: *** [ns4xPlugin.obj] Error 2

Comment 14

10 years ago
Comment on attachment 276974 [details] [diff] [review]
Backup. This compiles on Mac.

nsScriptablePeer.cpp(85) : error C2039: 'nsIScriptable' : is not a member of 'operator``global namespace'''
make[5]: *** [nsScriptablePeer.obj] Error 2
(Assignee)

Comment 15

10 years ago
Oooh, thanks Neil.

Changed all kPluginManagerCIDs in ns4xPlugin.cpp to kCPluginManagerCID, and fixed all three copies of that nsScriptablePeer to have nsIScriptable be nsIScriptablePluginSample.

Comment 16

10 years ago
Comment on attachment 276974 [details] [diff] [review]
Backup. This compiles on Mac.

nsOEImport.cpp(589) : error C2065: 'kISupportsIID' : undeclared identifier
make[4]: *** [nsOEImport.obj] Error 2
(Assignee)

Comment 17

10 years ago
Fixed, dropped the QI to nsISupports and instead I'm just AppendElement()ing pID directly.
(Assignee)

Comment 18

10 years ago
Created attachment 277140 [details] [diff] [review]
Backup. This compiles on all the Try servers and Neil's Windows box.
Attachment #276974 - Attachment is obsolete: true
Priority: P1 → P3
Target Milestone: --- → Future
You need to log in before you can comment on or make changes to this bug.