92377 - Build failure: dom/src/base/nsDOMClassInfo.cpp depends on extensions/xmlextras

Status: VERIFIED FIXED

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

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. Disable extension xmlextras
2. Build

Actual result:
Build fails in dom/src/base/nsDOMClassInfo.cpp, because it #includes several
interfaces from extensions/xmlextras. Compiler output below.

Expected result:
Build succeeds, no traces of xmlextras.

Additional Comments:
- This is a regression caused by bug 83433.
- Beonex Communicator does not ship with xmlextras. We're close to a release.
  Not sure, what to do. Any idea about a workaround (/hack)?


c++ -o nsDOMClassInfo.o -c -DOSTYPE=\"Linux2.2\" -DOSARCH=\"Linux\" -DOJI
-D_IMPL_NS_DOM   -I../../../dist/include -I../../../dist/include
-I/usr/src/beonex-0.7/bin-i686/dist/include/nspr      -I/usr/X11R6/include  
-fPIC  -mcpu=pentiumpro -march=pentiumpro  -I/usr/X11R6/include -fno-rtti
-fno-exceptions -Wall -Wconversion -Wpointer-arith -Wbad-function-cast
-Wcast-align -Woverloaded-virtual -Wsynth -pedantic -Wno-long-long
-mcpu=pentiumpro -march=pentiumpro -pipe -pthread -O2  -DNDEBUG -DTRIMMED
-I../../../../mozilla/dom/src/base/../build -mcpu=pentiumpro -march=pentiumpro 
-I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../../config-defs.h
-Wp,-MD,.deps/nsDOMClassInfo.pp ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:237: nsIXMLHttpRequest.h: No
such file or directory
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:238: nsIDOMSerializer.h: No
such file or directory
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:239: nsIDOMParser.h: No such
file or directory
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp: In function `static
nsresult nsDOMClassInfo::Init()':
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1505: `nsIXMLHttpRequest'
undeclared (first use this function)
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1505: (Each undeclared
identifier is reported only once
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1505: for each function it
appears in.)
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1505: template argument 1 is
invalid
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1506: `nsIJSXMLHttpRequest'
undeclared (first use this function)
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1506: template argument 1 is
invalid
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1511: `nsIDOMSerializer'
undeclared (first use this function)
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1511: template argument 1 is
invalid
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1515: `nsIDOMParser'
undeclared (first use this function)
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:1515: template argument 1 is
invalid
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp: In method `nsresult
nsStringArraySH::GetProperty(nsIXPConnectWrappedNative *, JSContext *, JSObject
*, long int, jsval *, PRBool *)':
../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4429: warning: unused
variable `PRInt32 n'
make[3]: *** [nsDOMClassInfo.o] Error 1
make[3]: Leaving directory `/mnt/data/compile/beonex-0.7/bin-i686/dom/src/base'
make[2]: *** [install] Error 2
make[2]: Leaving directory `/mnt/data/compile/beonex-0.7/bin-i686/dom/src'
make[1]: *** [install] Error 2
make[1]: Leaving directory `/mnt/data/compile/beonex-0.7/bin-i686/dom'
make: *** [install] Error 2

Comment 1

[Ben Bucksch (:BenB)]

> Any idea about a workaround (/hack)?

Apart from backing out the change :).

Comment 2

[Johnny Stenback (:jst)]

Over to peterv who's already working on this...

Comment 3

[Ben Bucksch (:BenB)]

Any ETA?

Comment 4

[Peter Van der Beken [:peterv]]

No ETA yet, sorry. I'm still trying to find out what would be involved in fixing
this.

Comment 5

[Johnny Stenback (:jst)]

Peter, well, if this is important to someone we could always temporarily put
#ifdef XSLT or whatever around the xslt code in nsDOMClassInfo.cpp...

Comment 6

[Peter Van der Beken [:peterv]]

Yeah, except we don't have a define for XSLT anymore. Ditto for XML Extras
(which seems to be the one giving Beonex grieve). I wonder if people could just
build with XML Extras/Transformiix and not package them. How would the DOM class
info stuff handle that?

Comment 7

[Ben Bucksch (:BenB)]

> we could always temporarily put
> #ifdef XSLT or whatever around the xslt code in nsDOMClassInfo.cpp...

Is it just nsDOMClassInfo.cpp? Can I just back out the change in
nsDOMClassInfo.[cpp|h] only?

(What was giving me grieve was
- the large number of files
- the fact that other checkins happened after the checkin
- the branching
.)

Comment 8

[Johnny Stenback (:jst)]

You can not back those changes out. If this is such a problem for you then just
comment out the XSLT specific code in nsDOMClassInfo.cpp and nsIDOMClassInfo.h
and you should be all set.

Comment 9

[cls]

Welcome to bug 18352 redux.  The bits of the main application should not depend
upon an extension.  Ok, technically, it only depends upon the header. 
Unfortunately, that still is a no-no because of the way we build.  So, it's
obvious to me that we need a better separation between church and state...err,
interfaces and implementations.  This is where bug 59454 comes in. 

Comment 10

[Peter Van der Beken [:peterv]]

Apparently Vidur is working on something similar. I'll try to get in touch with
him. Ben Bucksch, I'll attach a patch to disable the XML Extras stuff in the
DOMClassInfo files. That should get your builds going again for now.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Temporary workaround for disabling XML Extras

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't really _want_ this bug, but I think I made the mistake of getting the
most work on it done so far, and admitting it in public.  When will I learn?

Comment 13

[Brendan Eich [:brendan]]

How's it going?  We're getting closer to the Feb 1 proposed cutoff.  peterv,
shouldn't you take this bug back from shaver?

/be

Comment 14

[Peter Van der Beken [:peterv]]

Ok, too bad we didn't see the patch from shaver. I'll take this back and start
working on it.

Comment 15

[Ben Bucksch (:BenB)]

Attachment: Updated workaround

Comment 16

[Johnny Stenback (:jst)]

A real patch is on its way for this...

Comment 17

[Brendan Eich [:brendan]]

Any news?  We're past the deadline (01-Feb-2002).

/be

Comment 18

[Peter Van der Beken [:peterv]]

I'm going to post later what I am implementing but I still need some time. I
could supply a patch which disables DOMClassInfo for the Transformiix and
XMLExtras classes but implements regular ClassInfo, which would break the
dependency without disabling those modules completely. I'm not completely sure
what exactly I would be breaking. I'm currently compiling a tree that has such a
patch to see what happens.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Better workaround

This should enable building without extensions. It's a workaround until my real
fix to nsDOMClassInfo is ready. I've tried the XMLExtras tests and the
Transformiix test and everything seems to work. One part that doesn't work is
the XPath NodeSets, I can't supply ClassInfo for them so users from JS will
probably have some trouble. I doubt anybody uses those right now and they
should be replaced soon by my DOM Level 3 XPath implementation.
I still need to test this patch on Linux and Windows, will do that tomorrow.
Shall we check in this one and remove the workaround once the real patch goes
in? It would avoid disabling Transformiix and XMLExtras completely in the
default build.

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: v1

I wrote a description of what I changed, I'll post it tomorrow after I finish
it. Still have to try this out on windows and linux.

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: v2

New patch clears up an ownership issue for the nsDOMClassInfo we create for
external objects.

Comment 22

[Peter Van der Beken [:peterv]]

I think this patch is ready for reviewing.
This patch provides three things:
- a mechanism for modules other than DOM to register classes by their JS name
and have DOMClassInfo and .prototype magic.
- a generic scriptable helper object for modules who only need basic DOMClassInfo.
- .prototype magic for external constructors with the same name as the classes
they construct.

Modules other than the DOM module can now register two new categories:
JAVASCRIPT_DOM_CLASS, taking a contractid for a 'registrar' class, and
JAVASCRIPT_DOM_INTERFACE, taking an IID converted to a string. The DOM module
puts these classes and interfaces into its hashtable (hashed by name), and the
classes use a new type (eTypeExternalClassInfoCreator). When we try to resolve
one of these class names, we look in the hash table and if the struct has type
eTypeExternalClassInfoCreator, we try to instantiate the registrar class using
the contractid that was supplied and ask it to register the class info with the
DOM module. This causes the module that registered the class to get loaded (if
it hasn't already been loaded by something else). The registrar then calls back
into the DOM module (through the nsDOMSOFactory) and supplies the info to create
a nsDOMClassInfo structure which gets put into the hashtable replacing the
existing eTypeExternalClassInfoCreator with a eTypeExternalClassInfo. The
registrar can supply a constructor function that returns a scriptable helper (as
a nsIClassInfo*), if none is supplied we'll create an generic helper
(nsDOMGenericSH). The registrar can also supply the CID of the class, if this is
a class that needs a constructor with the same name (to make new Foo() work for
a class named Foo). Apart from that nsDOMClassInfo hasn't changed much.
The module can get a pointer to the scriptable helper by name from
nsDOMSOFactory, because it might need it when the class gets QI'ed to
nsIClassInfo. Because external modules could be holding on to the generic
scriptable helper we created, and that helper needs the nsDOMClassInfoData
structure, we can't just blindly delete the structure when the hashtable gets
destroyed. So we use the low bit of the mCachedClassInfo member to encode if the
scriptable helper is for an internal or an external class. The nsDOMClassInfo
destructor can then determine if it should delete the nsDOMClassInfoData
structure or not (internal classes have their nsDOMClassInfoData stored in a
static table).
I've supplied a few helper macros for modules that want the basic functionality
and converted Transformiix and XML Extras.
I hope that makes sense to anyone else.

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 68865 [details] [diff] [review]
v2

I like the patch so far (still reading it -- it's big!), but I'm a little
nervous about RegisterExternalClasses, which appears to be on the startup path.
 Have you run Ts/Tp/Txul numbers on this patch, so we can see if we're going to
have stress when it hits the tinderboxes?

(I wonder if we'll build xmlextras/transformiix in the default tinderbox
performance config after they become optional.	Perhaps not, in which case that
won't matter much.)

Comment 24

[Johnny Stenback (:jst)]

- nsIDOMClassInfo.h uses mixed 2 and 4 space indentation. Stick with 2 space
indentation.

- In nsDOMClassInfo::~nsDOMClassInfo(), 4 space indentation.

- nsDOMClassInfo::RegisterExternalClasses() is an awefully long function, could
we split it up and maybe share some of the code...?

- In StubConstructor(), the cases where we *might* fail for valid reasons (like
components being unregisterd since we started up) we should throw an exception
and not just return JS_FALSE. Just returning JS_FALSE is a bit dangerous since
it results in silent interruption of the script.

- At around line 3300 in nsDOMClassInfo.cpp:

+    NS_ASSERTION(name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo,
+                 "The classinfo data for this class didn't get registered.");
+
+    if (name_struct->mType != nsGlobalNameStruct::eTypeExternalClassInfo) {
+      return NS_OK;
+    }

How about this in stead:

+    if (name_struct->mType != nsGlobalNameStruct::eTypeExternalClassInfo) {
+      NS_ERROR("The classinfo data for this class didn't get registered.");
+
+      return NS_OK;
+    }

- Next hunk:

+    JSFunction *cfnc;
+    if (name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo &&
+      name_struct->mData->mConstructorCID) {
+      cfnc = ::JS_DefineFunction(cx, obj,
+                                 NS_ConvertUCS2toUTF8(name).get(),
+                                 StubConstructor, 0, 0);
+    } else {
+      cfnc = ::JS_DefineFunction(cx, obj,
+                                 NS_ConvertUCS2toUTF8(name).get(),
+                                 NativeConstructor, 0, 0);
+    }
+

How about this in stead:

+    JSFunction *cfnc;
+    JSNative *native;
+    if (name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo &&
+      name_struct->mData->mConstructorCID) {
+      native = StubConstructor;
+    } else {
+      native = NativeConstructor;
+    }
+
+    cfnc = ::JS_DefineFunction(cx, obj,
+                               NS_ConvertUCS2toUTF8(name).get(),
+                               native, 0, 0);
+    }

- In nsDOMClassInfo.h, CLEAN_CI_PTR should IMO be renamed to GET_CLEAN_CI_PTR.
Also, do we really need both IS_INTERNAL and IS_EXTERNAL? I guess I don't really
care, but I would've defined only one of them.

- We might want to change the type of the aName argument to
nsDOMSOFactory::GetExternalClassInfoInstance() from const char* to a const
nsAString& so that we don't need to do the ASCII to UCS2 conversion in that
function, the caller can do that conversion much cheaper than we can here.

- Also in nsDOMSOFactory::GetExternalClassInfoInstance(), after calling
creator->RegusterClassInfo(), you should look up globalStruct again before
touching it since it might have been moved around in the pldhash inside the
namespace manager.

- In GlobalNameHashClearEntry(), put braces around the one line if statement,
and use 2 space indentation.

- In nsScriptNameSpaceManager::RegisterExternalClassName(), could you not pass
the nsCID by reference and not by value?

- It should be documented somewhere that the name that's passed to
nsScriptNameSpaceManager::RegisterClassInfoData() must be static data, it will
not be deleted by the DOM code.

Other than that I say sr=jst.

Comment 25

[Peter Van der Beken [:peterv]]

Shaver: I was going to run the performance tests to see if there's a difference.
Of course, only having XMLExtras and Transformiix now might minimize it. If
other classes would be added later, it might still grow. I don't think we can
really avoid it though, we need to register the external classes at some point.

Comment 26

[Peter Van der Beken [:peterv]]

Ok, I got the numbers and they're going up :-(.

           Without  With
           patch    patch
Ts Min      2253     2274
Ts Max      2327     2336
Ts Average  2281     2293
Txul         621      627
Tp          1304     1318

I'm going to attach a new patch that resolves jst's comments, and then I'll try
to figure out if there's a way to optimize more.

Comment 27

[Johnny Stenback (:jst)]

Did those numbers come from a one run only test, or are they best of 3 or
something? I'm questioning the accuracy of the tests here...

Comment 28

[Peter Van der Beken [:peterv]]

Thanks for the help Johnny ;-). Txul was run once, but it opens 10 windows. Tp
was run once but did 5 iterations. Ts was run 10 times.

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: review comments from v2

I did a review of v2 on the plane, these are the results.  I was kinda tired,
so be gentle.

There are some performance titbits in there, I think.  At least, I thought that
when I was writing it.

Comment 30

[Johnny Stenback (:jst)]

Comments about shaver's comments:

Re: the footprint effects of this change:

-  const nsDOMClassInfoID mID;
+  const nsDOMClassInfoData* mData;

This changes nothing in terms of footprint except for the external class cases
where the nsDOMClassInfoData struct is allocated. For the internal classes there
is no change, mData will point to the static data in nsDOMClassInfo.cpp...

Re: GlobalResolve() growing and performance, we're still down to one single
string based pldhash lookup per resolve call (plus the old additional fluff),
this patch changes nothing in that respect. The only change here is when
resolving a class name for the first time in a scope, which obviously happens
very rarely and would hardly impact DHTML performance noticeably.

Re: DOM typelibs and constant counts:

The reason we do this is to get things like nsIDOMNodeFilter to appear as
NodeFilter on all scopes since it's needed for accessing NodeFilter.SKIP_ALL,
NodeFilter.SKIP_... The only other possibility here with the current code is to
hardcode the names of all the interfaces that have constants in them (and
doesn't appear on any class' proto chain) in the nsDOMCI code, and that seems
like a really bad and maintenance costly approach to me (and fragile too). Yes,
this does pull in a few xpt files that otherwise wouldn't be loaded in a normal
browsing session, but I don't know for sure what the lifetime of the heap hit we
take for doing that is since once we're done enumerating those interfaces, we'll
realease all the xpt related references. Jband would know.

Either way, this is nothing new, it's been in the code since the XPCDOM landing,
and it was even worse in the early days until jband hacked in more nifty
filtering support into the xpt enumeration code, so if we need to do something
more about this, we should file a separate bug on this and let this patch in.

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: v3

I think I fixed all the comments except for the naming problem (nsI* and
nsIDOM*). I think solving that will not be easy, I'd like to file it as a
separate bug. I redid perf tests: the Txul and Tp numbers are the same (within
2ms difference), the Ts is still slightly up (2330 vs 2313). (Here's me hoping
I didn't do anything stupid!)

Comment 32

[Peter Van der Beken [:peterv]]

>- nsIDOMClassInfo.h uses mixed 2 and 4 space indentation. Stick with 2 space
>indentation.

Done.

>- In nsDOMClassInfo::~nsDOMClassInfo(), 4 space indentation.

Done.

>- nsDOMClassInfo::RegisterExternalClasses() is an awefully long function, could
>we split it up and maybe share some of the code...?

Moved the interface registration to
nsScriptNameSpaceManager::RegisterExternalInterfaces and call that from
nsDOMClassInfo::RegisterExternalClasses.

>- In StubConstructor(), the cases where we *might* fail for valid reasons (like
>components being unregisterd since we started up) we should throw an exception
>and not just return JS_FALSE. Just returning JS_FALSE is a bit dangerous since
>it results in silent interruption of the script.

Made errors throw exeptions.

>- At around line 3300 in nsDOMClassInfo.cpp:
>
>+    NS_ASSERTION(name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo,
>+                 "The classinfo data for this class didn't get registered.");
>+
>+    if (name_struct->mType != nsGlobalNameStruct::eTypeExternalClassInfo) {
>+      return NS_OK;
>+    }
>
>How about this in stead:
>
>+    if (name_struct->mType != nsGlobalNameStruct::eTypeExternalClassInfo) {
>+      NS_ERROR("The classinfo data for this class didn't get registered.");
>+
>+      return NS_OK;
>+    }

Done.

>- Next hunk:
>
>+    JSFunction *cfnc;
>+    if (name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo &&
>+      name_struct->mData->mConstructorCID) {
>+      cfnc = ::JS_DefineFunction(cx, obj,
>+                                 NS_ConvertUCS2toUTF8(name).get(),
>+                                 StubConstructor, 0, 0);
>+    } else {
>+      cfnc = ::JS_DefineFunction(cx, obj,
>+                                 NS_ConvertUCS2toUTF8(name).get(),
>+                                 NativeConstructor, 0, 0);
>+    }
>+
>
>How about this in stead:
>
>+    JSFunction *cfnc;
>+    JSNative *native;
>+    if (name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo &&
>+      name_struct->mData->mConstructorCID) {
>+      native = StubConstructor;
>+    } else {
>+      native = NativeConstructor;
>+    }
>+
>+    cfnc = ::JS_DefineFunction(cx, obj,
>+                               NS_ConvertUCS2toUTF8(name).get(),
>+                               native, 0, 0);
>+    }

Done.

>- In nsDOMClassInfo.h, CLEAN_CI_PTR should IMO be renamed to GET_CLEAN_CI_PTR.
>Also, do we really need both IS_INTERNAL and IS_EXTERNAL? I guess I don't really
>care, but I would've defined only one of them.

Done.

>- We might want to change the type of the aName argument to
>nsDOMSOFactory::GetExternalClassInfoInstance() from const char* to a const
>nsAString& so that we don't need to do the ASCII to UCS2 conversion in that
>function, the caller can do that conversion much cheaper than we can here.

Done.

>- Also in nsDOMSOFactory::GetExternalClassInfoInstance(), after calling
>creator->RegusterClassInfo(), you should look up globalStruct again before
>touching it since it might have been moved around in the pldhash inside the
>namespace manager.

Done. Also done in the two other places where we call RegisterClassInfo and then
use the struct again.

>- In GlobalNameHashClearEntry(), put braces around the one line if statement,
>and use 2 space indentation.

Done.

>- In nsScriptNameSpaceManager::RegisterExternalClassName(), could you not pass
>the nsCID by reference and not by value?

Done.

>- It should be documented somewhere that the name that's passed to
>nsScriptNameSpaceManager::RegisterClassInfoData() must be static data, it will
>not be deleted by the DOM code.

Done. Also added the comment to nsIDOMScriptObjectFactory::RegisterDOMClassInfo.

>nsIDOMClassInfo.h:
>
> * the macro impl of RegisterClassInfo exports the CID of the DOM script
>   object factory -- that should probably use the ContractID.  But then,
>   can the caller provide the nsIDOMScriptObjectFactory more cheaply?  It's
>   not clear if nsWindowSH has easy access to the nsIDSOF handle, but you
>   guys probably know.  Certainly, the call from the nsDOMSOFactory itself
>   has the pointer around, so the macro could perhaps fetch the service only
>   if the caller doesn't have it at hand (and therefore passes nsnull for
>   the hypothetical aDOMSOFactory argument).

I've made the callers always pass in the nsIDOMScriptObjectFactory.

>(@@ -3201,10 +3320,20 @@ stanza)
>+    if (name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo &&
>+      name_struct->mData->mConstructorCID) {
>+      cfnc = ::JS_DefineFunction(cx, obj,
>+                                 NS_ConvertUCS2toUTF8(name).get(),
>+                                 StubConstructor, 0, 0);
>+    } else {
>+      cfnc = ::JS_DefineFunction(cx, obj,
>+                                 NS_ConvertUCS2toUTF8(name).get(),
>+                                 NativeConstructor, 0, 0);
>+    }
>+
>
> * Why do we use UTF8 here?  Above (line 2990, pre-patch) we bemoan the
>   absence of JS_DefineUCFunction, and then deflate to ASCII via
>   JS_GetStringBytes(str).  I think we want to do the same here, until
>   we remedy this i18n-unfriendly JSAPI lack.

Done.

> * (Is there a bug filed on the lack of JS_DefineUCFunction?)

Jst? (He's the one bemoaning ;-))

>@@ -87,11 +127,12 @@
>   static nsresult ThrowJSException(JSContext *cx, nsresult aResult);
> 
> protected:
>-  const nsDOMClassInfoID mID;
>+  const nsDOMClassInfoData* mData;
> 
> * Hrm.  What are the footprint effects of this change?  It looks like we've
>   added an additional allocation of 7 words (8 with malloc slop, likely) to
>   every nsDOMCI object.  Do we have a lot of those?

Jst answered this one.

>+    if (IS_EXTERNAL(mData->mCachedClassInfo))
>+        delete (nsExternalDOMClassInfoData*)mData;
>
> * NS_STATIC_CAST, please!

Done.

>+
>+      // Make sure we don't leak this error code to the caller.
>+      rv = NS_OK;
>+
>
> * How could that error code leak to the caller?  I don't see an exit path
>   from that method that doesn't reset rv: either continue runs the loop body
>   again, and we set rv to the return value of cm->GetData(...), or we're
>   on the last iteration and we set it to the return value of
>   cm->EnumerateCategory below.

Removed the comment.

>+      nsIID *iid = nsnull;
>+
>+      if_info->GetIID(&iid);
>[...]
>+      nsXPIDLCString name;
>+      if_info->GetName(getter_Copies(name));
>[...]
>+      nsMemory::Free(iid);
>+
>
> * Can nsIInterfaceInfo::getIIDShared and ::getNameShared save us some
>   allocations and copies?

Switched to non-copying versions.

>+    cm->GetCategoryEntry(JAVASCRIPT_DOM_INTERFACE, categoryEntry,
>+                         getter_Copies(IID_string));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsIID primary_iid;
>+    if (!primary_iid.Parse(IID_string) ||
>
> * Man, I wish we could store IIDs in category entries, rather than having to
>   parse every time.  Alas.

Yeah, alas.

> * We were already dealing in nsDOMCIData structures before this patch, of
>   course, but I'd still like some reassurance that we're not going to get
>   stung too badly by this part of the switch.  (I appreciate that some
>   space and/or speed cost is inevitable: you're adding a level of indirection,
>   and indirection isn't free.  I just want to hear you say "we only create a
>   few dozen of these things, and we were already creating these nsDOMCIData
>   things before, just a little later", or something else equally comforting.)

We were already creating these nsDOMCIData things before, we just do it a little
later (and on demand!). ;-) See also jst's comments.

> * While on that topic, do we share nsDOMCIData structures, or do they otherwise
>   ever have a lifetime that doesn't match that of the referring nsDOMCI
>   wrappers?  If not, we can nest a structure rather than using an out-of-line
>   pointer with separate allocation.

They don't outlive the wrappers, but we can have a nsDOMCIData struct without a
nsDOMCI wrapper.

>+      if (nsCRT::strncmp(if_name, NS_DOM_INTERFACE_PREFIX,
>+                         (PRUint32)strlen(NS_DOM_INTERFACE_PREFIX)) == 0) {
>
> * nsCRT::strncmp is deprecated (and dangerous, because of possibly-ambiguous
>   overload resolution, owing to the fact that the two versions differ only
>   in the signedness of their final parameter), and adds no value.  "Bare" 
>   strncmp is to be preferred for |char *| (ASCII, ISO-8859-1, UTF-8, etc.) 
>   strings.

Done.

>+      rv = RegisterInterface(if_info, if_name.get() +
strlen(NS_INTERFACE_PREFIX), &found_old);
>
> * No need to compute strlen of a constant string at runtime, when we have
>   the compile-time sizeof.

Done.

> * But do we really want to force that naming on external DOM entries?  It would
>   certainly make it harder to use this mechanism for implementing standards
>   prescribed script-thing names: if we're supposed to register "Request" in the
>   global namespace, we're going to bang heads with the "real" nsIRequest.
>   How about "domI" or "domextI" as a prefix that is unlikely to cause
>   collision (unless, of course, two extensions use the same interface...)?
>
>   Or maybe overloading interface name for script-thing name is just the wrong
>   path, and we need to explicitly parameterize that?

I'd like to push this out to a new bug. From what I can see this would involve a
second mapping (real interface name to JS name) for when we walk up the
interfaces of a class.

> * DeBrendan's theorem, or something:
>   NS_ASSERTION(s->mType == ::eTypeNotInitialized || s->mType ::eTypeInterface,
>                "Whaaa, ...");
>
>+    { "Transformiix DOMCI Registrar",
>+      TRANSFORMIIX_DOMCI_REGISTRAR_CID,
>+      TRANSFORMIIX_DOMCI_REGISTRAR_CONTRACTID,
>+      NS_DOMCI_REGISTRAR_CONSTRUCTOR(Transformiix) }

Done.

> * Hmm, now I understand jst's objection to the "registrar" name.  Unless I've
>   been living a lie, a registrar is an entity with which one registers
>   something, and not an entity requesting or performing registration.
>   "Registerer" is not going to fly, clearly.  How about just "extension" or
>   "hook"?  "Transformiix DOMCI Extender"?

Extension it is, and Extender.

> * We should file a bug on the SVG folks, too, so they can move to the new
>   mechanism when the dust settles.

Yes. So when will we have extensible layout? Drop in the SVG component. Hmmm...

Comment 33

[Johnny Stenback (:jst)]

Comment on attachment 70853 [details] [diff] [review]
v3

- Document the fact that that the primary interface must be registerd for
protos of a class to be accessable.

- Ignore attempts at registering classes with non-ASCII characters in the class
name in nsScriptNameSpaceManager::RegisterClassName()

With that, sr=jst

Comment 34

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 70853 [details] [diff] [review]
v3

+  NS_ASSERTION(sizeof(PtrBits) == sizeof(void*),
+		"BAD! You'll need to adjust the size of PtrBits to the size "
+		"of a pointer on your platform.");
+

I'd rather see that asserted at compile time:

#if sizeof(whatever PtrBits is) != sizeof(void *)
#error "PtrBits must have the same size as a pointer"
#endif

Fix that baby up, and r=shaver. Let's get this baby in.

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Forget that.  #if can't do sizeof.  Bah.

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 70853 [details] [diff] [review]
v3

a=asa (on behalf of drivers) for checkin to 0.9.9 and the trunk.

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: final patch

This is the patch I checked in into the branch, containing the last-minute
changes that went in to the trunk. Thanks to shaver for late-night bustage
fixing.

Comment 38

[Peter Van der Beken [:peterv]]

Fixed in trunk and branch, disable-extensions should work now.

Comment 39

[Eric Murphy]

parseFromString is no longer working. This seems to have broken it.

See http://bugzilla.mozilla.org/show_bug.cgi?id=129776

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Please file a new bug for fallout like that.  This bug is huge, old, and about a
different thing.

Comment 41

[Sivakiran Tummala]

verified.