236408 - deCOMtaminate nsINodeInfoManager

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There is currently one user of nsINodeInfoManager outside of gklayout.so, which
is in editor:

http://lxr.mozilla.org/mozilla/ident?i=nsINodeInfoManager

Though if the patch in bug 221335 gets checked in we'd have two. We could either
keep that function virtual, or we could add a function like:

nsIDocument::createElem(PRInt32, nsIAtom*, nsIAtom*, nsIContent**);

This would probably clean up both the editorclient and the transformiix one. We
might need to do something about the inline callers in nsINodeInfo that uses
mOwnerManager, but I think most of those live in content already.

If that is done everyone could just use nsNodeInfoManager directly and we could
get rid of the nsINodeInfoManager.

Comment 1

[Peter Van der Beken [:peterv]]

I already have a patch for this.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sweet :)

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 148619 [details] [diff] [review]
v1.1

I'd especially like feedback on the fact that nsHTMLDocument::CreateElement
creates nodes with kNameSpaceID_None if passed kNameSpaceID_XHTML and the
document is not XHTML. I did it that way to avoid forcing the XHTML check on
the callers.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Peter, I won't be able to look at this until at least Wednesday...  :(

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

comment 7 sounds scary to me, but i havn't looked at the patch so I can't 
express a too strong oppinion as is

Comment 10

[Peter Van der Beken [:peterv]]

Well, we can always have them pass in another namespace ID, to denote the type
of element to create.
Look for example at the editor, right now they use only HTML documents. Imagine
they started supporting XHTML (far-fetched, I know ;-)), they want to create an
(X)HTML element, but they would need to figure out if the document is HTML or
XHTML and pass in a different namespace ID depending on that. I'm not sure what
the best solution is, but the two use-cases we have (editor and transformiix)
would both do exactly that, so I figured we might as well move it inside the
document.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't they both need to know this anyway? Transformiix behaves differently for 
xhtml and html, and i would guess that editor will too...

Comment 12

[Peter Van der Beken [:peterv]]

Fine, I'll post a new patch (don't really care either way).

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1.2

CreateElement now takes an aElementType argument.

Comment 14

[Peter Van der Beken [:peterv]]

Bah, I forgot to ifdef DEBUG the declaration of CreateElement in nsHTMLDocument,
so this patch won't compile as is in an optimized build. The fix is trivial, but
it leads to
virtual nsresult nsDocument::CreateElement(nsIAtom*, nsIAtom*, int, int,
nsIContent**)' was hidden by `virtual nsresult
nsHTMLDocument::CreateElement(const nsAString&, nsIDOMElement**)'
Anyone have a better name that doesn't conflict with CreateElement?

Comment 15

[Boris Zbarsky [:bzbarsky]]

We're eliminating element factories, right?  It looks like we're going to just
having a hardcoded list of content nodes that we know about.  What are the
benefits and drawbacks of that?  I recall seeing some discussion on that
recently, but can't remember where...

Why have NS_NewHTMLDivElement, NS_NewHTMLHRElement, NS_NewHTMLImageElement
exposed like that?  Can't NS_NewHTMLElement handle those cases?  What is the
case-sensitivity of the nodes those functions produce anyway?  That should
probably be documented.  Alternately, why is NS_NewHTMLElement not just
determining case-sensitivity itself based on the nodeinfo?  Do we think that
passing around the extra param (esp. from all the HTML content sink methods) is
faster, in general?

Why not deCOMify GetDocumentPrincipal()?

In nsIDocument, "[OWNING REF]" may be clearer than "[OWNS]", maybe?  The latter
implies that destructor will call |delete|, not |NS_RELEASE|, to me.

The decl of NS_NewElement in nsContentCreatorFunctions.h should probably use
aElementType instead of aElementID....

More comments (after closer reading) later; I'm interested in the element
factory story first.

Also, do we plan to deCOMify nsINodeInfo as well?

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to comment #15)
> We're eliminating element factories, right?  It looks like we're going to just
> having a hardcoded list of content nodes that we know about.  What are the
> benefits and drawbacks of that?  I recall seeing some discussion on that
> recently, but can't remember where...

There's two parts to this. On the provider side the drawback is simple, no way
for someone other than Gecko to provide an element factory and its own element
implementation for a specific namespace. The discussion you recall is about XTF
which has its own element factories and was wrapping them in nsIElementFactory.
IIRC those wrappers are inside Gecko though, so XTF could just store an array of
its own element factories instead. We could keep them if people feel it's
important functionality, we wouldn't use them at all except if someone adds an
element implementation outside Gecko. On the consumer side, I think it doesn't
make sense to use them inside Gecko for elements that we know Gecko provides.
There was one caller outside Gecko (and one patch adding a caller) and they both
already have a document. I think it makes more sense to have them get the
element through that document.

> Why have NS_NewHTMLDivElement, NS_NewHTMLHRElement, NS_NewHTMLImageElement
> exposed like that?  Can't NS_NewHTMLElement handle those cases?

Sure, but I tend to prefer to call the right function immediately. You know
you're creating an XHTML div element, so that's what you create. I know
NS_NewHTMLElement does more than just create the element (it ends up calling
SetForm in MakeContentObject, but aForm will be null anyway), so I could be
persuaded otherwise ;-).

> What is the
> case-sensitivity of the nodes those functions produce anyway?  That should
> probably be documented.

For NS_NewElement it depends on the aCaseSensitive parameter. Either we leave
the case alone, or we normalize it (through
nsIParserService::HTMLIdToStringTag). For the others we leave the case alone.
Hmm, good point, I think some callers want HTML nodes. I guess that points to
NS_NewHTMLElement after all.

> Alternately, why is NS_NewHTMLElement not just
> determining case-sensitivity itself based on the nodeinfo?  Do we think that
> passing around the extra param (esp. from all the HTML content sink methods) is
> faster, in general?

We could call aNodeInfo->NamespaceEquals(kNameSpaceID_XHTML) in
NS_NewHTMLElement and eliminate the argument.

> Why not deCOMify GetDocumentPrincipal()?

Because this patch isn't about deCOMtaminating nsINodeInfo.

> In nsIDocument, "[OWNING REF]" may be clearer than "[OWNS]", maybe?  The latter
> implies that destructor will call |delete|, not |NS_RELEASE|, to me.

Yes.

> The decl of NS_NewElement in nsContentCreatorFunctions.h should probably use
> aElementType instead of aElementID....

I see no aElementID argument?

> Also, do we plan to deCOMify nsINodeInfo as well?

It has been deCOMtaminated already (partially?), it's used outside Gecko though
so I don't think we easily can remove nsINodeInfo.

Comment 17

[Peter Van der Beken [:peterv]]

(In reply to comment #16)
> (In reply to comment #15)
> > Why not deCOMify GetDocumentPrincipal()?
> 
> Because this patch isn't about deCOMtaminating nsINodeInfo.

Nevermind, I forgot there's one in the manager too.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I see no aElementID argument?

Er, aNamespaceID.  The same argument is called aElementType where the function
is implemented, and that name makes more sense.

I agree that people creating elements should just create them off a document
object (which will incidentally make sure they always get the right owner document).

So yes, the only real question is whether we plan to have people implementing
new element types outside Gecko.  I guess it's OK to leave it impossible to do
that for now...

For case-sensitivity I meant things like NS_NewHTMLDivElement vs
NS_NewHTMLElement -- the latter has a case-sensitive arg, but the former does not.

Comment 19

[Peter Van der Beken [:peterv]]

I actually meant aElementType, thanks for catching that.
I've switched everything back to NS_NewHTMLElement and moved the
NS_NewHTMLDivElement, ... declarations back into nsGenericHTMLElement.h.

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: v1.3

I changed [OWNS] to [STRONG]

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 149418 [details] [diff] [review]
v1.3

Dude, rev some IID's here! :-)

- In nsIDocument.h:

+  nsNodeInfoManager* mNodeInfoManager; // [STRONG]

If so, then this class better do the cleanup of this pointer too. Maybe
uninline nsIDocument::~nsIDocument() and move that to nsDocument.cpp, and do
the cleanup there?

+#ifdef _IMPL_NS_LAYOUT
+  nsNodeInfoManager* GetNodeInfoManager() const
   {
     return mNodeInfoManager;
   }

That should be NodeInfoManager(), no? I.e. it'll never return null in a
non-broken case. And maybe loose the #ifdef _IMPL_NS_LAYOUT, just expose the
pointer, let people use it outside of layout if they really really want to
(just comment that they shouldn't, and kinda can't) :-)

+  /**
+   * Create an element. With the specified name, prefix and namespace ID.
+   * aElementType denotes the type of element to create, using the namespace
ID
+   * values (kNamespaceID_XHTML to create HTML elements, kNamespaceID_XUL to
+   * create XUL elements, ...).
+   */
+  virtual nsresult CreateElement(nsIAtom *aName, nsIAtom *aPrefix,
+				  PRInt32 aNamespaceID, PRInt32 aElementType,
+				  nsIContent** aResult) = 0;

How about changing aElementType to a PRBool, and naming it aDocumentDefaultType
(or something like that), and if called with that argument to false, just go
create an element based on the namespace ID in the nodeinfo, if true, create
the default element type for that document (i.e. a HTML element for a HTML
document, XUL for XUL, etc etc)? Sorry for not coughing up this idea earlier,
just didn't occure to me until now...

- In nsINodeInfo.h:

-  virtual ~nsINodeInfo() { }
+  virtual ~nsINodeInfo()
+  {
+  }

Couldn't we get rid of that alltogether, and make nsNodeInfo::~nsNodeInfo()
non-virtual?

+#ifdef _IMPL_NS_LAYOUT
   /*
    * Get the owning node info manager, this will never return null.
    */
-  nsINodeInfoManager* NodeInfoManager() const
+  nsNodeInfoManager *NodeInfoManager() const
   {
     return mOwnerManager;
   }
+#endif

Same here, do we need the #ifdef?

-  nsIDocument* GetDocument() const
-  {
-    return mOwnerManager->GetDocument();
-  }
+  virtual nsIDocument* GetDocument() const = 0;

Here it'd be cool to have one tho, and inline version of this (with a different
name) inside an #ifdef _IMPL_NS_LAYOUT.

- In nsNameSpaceManager.cpp:

+class nsNameSpaceEntry : public PLDHashEntryHdr

Shouldn't this inherit most of itself from PLDHashStringEntry? And who manages
the ownership of the strings in the keys? Some of the hash wrapper macros? (I
see you just moved this code, but still!)

+nsresult
+NS_NewElement(nsIContent** aResult, PRInt32 aElementType,
+	       nsINodeInfo* aNodeInfo)

If you make the change to nsIDocument::CreateElement(), you could probably
remove the aElementType argument from this method...

- In nsNodeInfoManager::AddRef():

+{
+  NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");
+
+  nsrefcnt count = PR_AtomicIncrement((PRInt32*)&mRefCnt);
+  NS_LOG_ADDREF(this, count, "nsNodeInfoManager", sizeof(*this));
+
+  return count;
+}

NS_IMPL_ADDREF(nsNodeInfoManager)? same for release...

That's all for now, next up, nsHTMLContentSink.cpp

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 149418 [details] [diff] [review]
v1.3

- In nsComboboxControlFrame::CreateAnonymousC():

+    NS_NewHTMLElement(getter_AddRefs(btnContent), nodeInfo);
+    NS_ENSURE_TRUE(btnContent, NS_ERROR_FAILURE);

Store the result in rv there and NS_ENSURE_SUCCESS(rv, rv)?

sr=jst with all that...

Comment 23

[Peter Van der Beken [:peterv]]

(In reply to comment #21)
> -  nsIDocument* GetDocument() const
> -  {
> -    return mOwnerManager->GetDocument();
> -  }
> +  virtual nsIDocument* GetDocument() const = 0;
> 
> Here it'd be cool to have one tho, and inline version of this (with a different
> name) inside an #ifdef _IMPL_NS_LAYOUT.

On a related note, do we want to define _IMPL_NS_LAYOUT in dom and view? They
both get into the layout library.

> - In nsNameSpaceManager.cpp:
> 
> +class nsNameSpaceEntry : public PLDHashEntryHdr
> 
> Shouldn't this inherit most of itself from PLDHashStringEntry? And who manages
> the ownership of the strings in the keys? Some of the hash wrapper macros? (I
> see you just moved this code, but still!)

mURIArray (for mapping ID -> URI) owns the strings. You'd store two strings
where one is enough :-).

> - In nsNodeInfoManager::AddRef():
> 
> +{
> +  NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");
> +
> +  nsrefcnt count = PR_AtomicIncrement((PRInt32*)&mRefCnt);
> +  NS_LOG_ADDREF(this, count, "nsNodeInfoManager", sizeof(*this));
> +
> +  return count;
> +}
> 
> NS_IMPL_ADDREF(nsNodeInfoManager)? same for release...

That would make them virtual. Maybe we should add some macros somewhere, I think
Transformiix has these too somewhere.

Comment 24

[Peter Van der Beken [:peterv]]

Question for jst: are you proposing that aDocumentDefaultType work for any
document? Would setting it to true create an SVG element in an SVG document? How
about for a MathML document (which doesn't have its own implementation)? I
wonder how useful that is for the API's users.
I'll add a GetDefaultElementType to all document implementations to implement this.

Comment 25

[Johnny Stenback (:jst)]

(In reply to comment #24)
> Question for jst: are you proposing that aDocumentDefaultType work for any
> document? Would setting it to true create an SVG element in an SVG document? How
> about for a MathML document (which doesn't have its own implementation)? I
> wonder how useful that is for the API's users.
> I'll add a GetDefaultElementType to all document implementations to implement
this.

I wasn't proposing to do that, no. I'd say that CreateElement() with
aDocumentDefaultType passed as true should create the default element type that
would be created if nsIDOMDocument::CreateElement() was called. IOW, for HTML,
an HTML element would be created, for XHTML, and XHTML element would be created,
for XUL, a XUL element would be created (since that's how we've "defined"
createElement() on XUL documents), but for SVG, MathML, and whatnot, a vanilla
XML element should be created since the specs for those types of documents state
that that's what happens.

It's not pretty, I know...

Comment 26

[Boris Zbarsky [:bzbarsky]]

It doesn't look like I'll have a chance to review this before I lose internet
access (and hence not before July 11 at the earliest).

Comment 27

[Peter Van der Beken [:peterv]]

Attachment: v1.4

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

I'll get to this on Monday.

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 150596 [details] [diff] [review]
v1.4

- In nsDocument::CreateElem():

+  PRInt32 elementType = aDocumentDefaultType ? GetDefaultElementType() :
+						aNamespaceID;

I wonder if it wouldn't be both faster and less code if we'd add a
mDefaultElementType member (could even be an 8-bit member since the namespace
ID's we care about are < 255) to nsDocument and make the appropriate document
classes set that in their ctors in stead of making this virtual call for every
element created? Not a big deal, just food for thought.

- In nsNodeInfoManager.cpp:

-NS_IMPL_THREADSAFE_ISUPPORTS1(nsNodeInfoManager, nsINodeInfoManager)
+nsrefcnt
+nsNodeInfoManager::AddRef()
+{
+  NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");

+  nsrefcnt count = PR_AtomicIncrement((PRInt32*)&mRefCnt);
+  NS_LOG_ADDREF(this, count, "nsNodeInfoManager", sizeof(*this));

-// nsINodeInfoManager
+  return count;
+}

Any reason not to use NS_IMPL_THREADSAFE_ADDREF() here? Same for ::Release().

sr=jst

Comment 30

[Peter Van der Beken [:peterv]]

(In reply to comment #29)
> I wonder if it wouldn't be both faster and less code if we'd add a
> mDefaultElementType member (could even be an 8-bit member since the namespace
> ID's we care about are < 255) to nsDocument and make the appropriate document
> classes set that in their ctors in stead of making this virtual call for every
> element created? Not a big deal, just food for thought.

Hmm, it doesn't really matter much either way I think. It's not for every
element created btw, just for those created through the DOM CreateElement method
or through the nsIContent CreateElem method.

> Any reason not to use NS_IMPL_THREADSAFE_ADDREF() here? Same for ::Release().

I don't want to have to declare them with NS_IMETHOD_ because that would make
them virtual (I suppose I could use NS_STDCALL but I'd rather not).

Comment 31

[Johnny Stenback (:jst)]

(In reply to comment #30)
> (In reply to comment #29)
> > I wonder if it wouldn't be both faster and less code if we'd add a
> > mDefaultElementType member (could even be an 8-bit member since the namespace
> > ID's we care about are < 255) to nsDocument and make the appropriate document
> > classes set that in their ctors in stead of making this virtual call for every
> > element created? Not a big deal, just food for thought.
> 
> Hmm, it doesn't really matter much either way I think. It's not for every
> element created btw, just for those created through the DOM CreateElement method
> or through the nsIContent CreateElem method.

Ah, ok. No big deal then, but it'd probably still be a bit less code :-) Your call.

> 
> > Any reason not to use NS_IMPL_THREADSAFE_ADDREF() here? Same for ::Release().
> 
> I don't want to have to declare them with NS_IMETHOD_ because that would make
> them virtual (I suppose I could use NS_STDCALL but I'd rather not).

Makes sense.

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 150596 [details] [diff] [review]
v1.4

(From update of attachment 150596 [details] [diff] [review])
>- content/base/public/nsContentUtils.h
>+  /**
>+   * Convenience method to create a new nsINodeInfo that differs only by name
>+   * from aNodeInfo.
>+   */
>+  static nsresult NameChanged(nsINodeInfo *aNodeInfo, nsIAtom *aName,
>+                              nsINodeInfo** aResult)

Nit: If you're going to use identifiers in your comment, do so consistently:
"that differs only by aName"...

>+  /**
>+   * Convenience method to create a new nsINodeInfo that differs only by prefix
>+   * from aNodeInfo.
>+   */
>+  static nsresult PrefixChanged(nsINodeInfo *aNodeInfo, nsIAtom *aPrefix,
>+                                nsINodeInfo** aResult)

Same here.

- In content/base/public/nsIDocument.h

>-  nsCOMPtr<nsINodeInfoManager> mNodeInfoManager;
>+  nsNodeInfoManager* mNodeInfoManager; // [STRONG]

How about turning this into:

  nsRefPtr<nsNodeInfoManager> mNodeInfoManager;

Same in nsINodeInfo (though I suppose you can feel free to turn these into a
separate bug if you -- understandably -- don't want to make another large
patch)

- In content/base/src/nsDocument.h

>   virtual PRInt32 GetDefaultNamespaceID() const
>   {
>     return kNameSpaceID_None;
>   };
>+  virtual PRInt32 GetDefaultElementType() const
>+  {
>+    return kNameSpaceID_None;
>+  };

Would it be worth adding a member to nsDocument for both of these to avoid the
virtual method overhead (which we will incur fairly often)?  The subclass
constructors would initialize the member to the correct value since they never
change.  Sure we'd slightly bloat documents, but we don't have too many
documents, really -- definitely not in comparison with elements.  It might be
worth investigating, but this does seem like something that is really begging
to NOT be virtual.  Magic 8-ball says you would get a code size win, if nothing
else (and have better looking source, IMO).

>+PRBool
>+NameSpaceManagerImpl::HasElementCreator(PRInt32 aNameSpaceID)
>+{
>+  return
>+#ifdef MOZ_MATHML
>+    aNameSpaceID == kNameSpaceID_MathML ||
>+#endif
>+#ifdef MOZ_XUL
>+    aNameSpaceID == kNameSpaceID_XUL ||
>+#endif
>+#ifdef MOZ_SVG
>+    aNameSpaceID == kNameSpaceID_SVG ||
>+#endif
>+    aNameSpaceID == kNameSpaceID_XHTML;
> }


How about putting XHTML first in the list?  To avoid the weird semi-colon
issue, you can do:

  return
    aNameSpaceID == kNameSpaceID_XHTML ||
#ifdef MOZ_MATHML
    aNameSpaceID == kNameSpaceID_MathML ||
#endif
    PR_FALSE;

>@@ -892,12 +893,12 @@ nsXULElement::CloneNode(PRBool aDeep, ns
>+        rv = NS_NewXULElement(getter_AddRefs(result), mSlots->mNodeInfo);
>+        if (NS_SUCCEEDED(rv)) {
>+            // XXX setting document on nodes not in a document so XBL will bind
>+            // and chrome won't break. Make XBL bind to document-less nodes!
>+            result->SetDocument(mDocument, PR_TRUE, PR_TRUE);
>+        }

Nit: That comment is a bit difficult to read as-is.  How about simplifying to:

  // XXX Make XBL bind to document-less nodes so chrome won't break!

Comment 33

[Peter Van der Beken [:peterv]]

(In reply to comment #32)
> - In content/base/public/nsIDocument.h
> 
> >-  nsCOMPtr<nsINodeInfoManager> mNodeInfoManager;
> >+  nsNodeInfoManager* mNodeInfoManager; // [STRONG]
> 
> How about turning this into:
> 
>   nsRefPtr<nsNodeInfoManager> mNodeInfoManager;
> 
> Same in nsINodeInfo (though I suppose you can feel free to turn these into a
> separate bug if you -- understandably -- don't want to make another large
> patch)

No can do, or I'd have to make the declaration of nsNodeInfoManager available to
nsIDocument.h and nsINodeInfo.h and we don't want to expose it outside Gecko.

Comment 34

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #33)
> (In reply to comment #32)
> > - In content/base/public/nsIDocument.h
> > 
> > >-  nsCOMPtr<nsINodeInfoManager> mNodeInfoManager;
> > >+  nsNodeInfoManager* mNodeInfoManager; // [STRONG]
> > 
> > How about turning this into:
> > 
> >   nsRefPtr<nsNodeInfoManager> mNodeInfoManager;
> > 
> > Same in nsINodeInfo (though I suppose you can feel free to turn these into a
> > separate bug if you -- understandably -- don't want to make another large
> > patch)
> 
> No can do, or I'd have to make the declaration of nsNodeInfoManager available to
> nsIDocument.h and nsINodeInfo.h and we don't want to expose it outside Gecko.
> 


You can simply forward declare nsRefPtr<> can't you?  You can at least for
nsCOMPtr<>.

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #34)
> You can simply forward declare nsRefPtr<> can't you?  You can at least for
> nsCOMPtr<>.

I meant of course, you can forward declare T and use it as nsRefPtr<T>

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, it won't work if the containing class (nsIDocument in this case) doesn't
have an explicit non-inlined ctor and dtor. Otherwise some compilers tries to
generate the ctor/dtor which will fail since it can't compile the ctor/dtor for
the member. Of course, a good compiler wouldn't generate the ctor/dtor unless
the class was actually instantiated, but some compilers do anyway.

Comment 37

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #36)
> No, it won't work if the containing class (nsIDocument in this case) doesn't
> have an explicit non-inlined ctor and dtor. Otherwise some compilers tries to
> generate the ctor/dtor which will fail since it can't compile the ctor/dtor for
> the member. Of course, a good compiler wouldn't generate the ctor/dtor unless
> the class was actually instantiated, but some compilers do anyway.

Oh.  Fair enough.  Which compilers?

Comment 38

[Peter Van der Beken [:peterv]]

Plus there's inline functions that use the pointer (see NodeInfoManager() for
example). Rest assured: I tried using nsRefPtr, I prefer them when possible.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't remember which compilers, i just remember getting red ports.

Comment 40

[Johnny Stenback (:jst)]

Hmm, wouldn't that be all compilers?

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #38)
> Plus there's inline functions that use the pointer (see NodeInfoManager() for
> example). Rest assured: I tried using nsRefPtr, I prefer them when possible.

shouldn't that continue to work anyway, since you don't need the entire
declaration to return a pointer?

(In reply to comment #36)
> No, it won't work if the containing class (nsIDocument in this case) doesn't
> have an explicit non-inlined ctor and dtor.

hm, what's the problem with adding an explicit ctor and dtor?

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Hmm, wouldn't that be all compilers?

No, IIRC some compilers (gcc3+ and/or msvc) only generate the ctor/dtor when
actually needed, i.e. when the class is instantiated or deleted. Like more or
less all compilers do with other inlined functions in templated classes.

> shouldn't that continue to work anyway, since you don't need the entire
> declaration to return a pointer?

You'll run into problems with nsRefPtr and nsCOMPtr not returning an actual
nsIFoo* but rather an nsDerivedSafe<nsIFoo>*. That class inherits nsIFoo which
will fail since nsIFoo is forward declared.

> hm, what's the problem with adding an explicit ctor and dtor?

We would have to add an nsIDocument.cpp (== ugly!!) since the ctor isn't allowed
to be inlined. If it is inlined the original problem remains.

Comment 43

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #42)
> > hm, what's the problem with adding an explicit ctor and dtor?
> 
> We would have to add an nsIDocument.cpp (== ugly!!) since the ctor isn't allowed
> to be inlined. If it is inlined the original problem remains.


Well, we _could_ stick it in nsDocument.cpp -- nsDocument is the only thing
which is a direct subclass of nsIDocument --  and I don't think that will be
changing any time soon.

Comment 44

[Peter Van der Beken [:peterv]]

Attachment: v1.5

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 151643 [details] [diff] [review]
v1.5

Carrying over r/sr.

Comment 46

[Johnny Stenback (:jst)]

This landed, right? Time to make this FIXED maybe? :-)

Comment 47

[Serge Gautherie (:sgautherie)]

(In reply to comment #46)
> This landed, right? Time to make this FIXED maybe? :-)

It seems so: { peterv%propagandism.org	2004-06-25 05:26 }.
And Target Milestone should be changed to v1.8a2.

Comment 48

[[e-mail]]

Comment on attachment 151643 [details] [diff] [review]
v1.5

>+#ifdef DEBUG
>+nsresult
>+nsHTMLDocument::CreateElem(nsIAtom *aName, nsIAtom *aPrefix,
>+                           PRInt32 aNamespaceID, PRBool aDocumentDefaultType,
>+                           nsIContent** aResult)
>+{
>+  NS_ASSERTION(!aDocumentDefaultType || IsXHTML() ||
>+               aNamespaceID == kNameSpaceID_None,
>+               "HTML elements in an HTML document should have "
>+               "kNamespaceID_None as their namespace ID.");
>+
>+  return nsDocument::CreateElement(aName, aPrefix, aNamespaceID,
>+                                   aDocumentDefaultType, aResult);
>+}
>+#endif
This caused bug 249665.

Comment 49

[Peter Van der Beken [:peterv]]

Closing now that I checked in the final piece (comments explaining a bustage fix).

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's not really good that we have both CreateElem and CreateElement that take
virtually the same arguments (PRBool and PRInt32 are both typedefed to 'int' on
32bit platforms) but do different things. I guess that's one of the things that
should be cleaned up with time.