Closed Bug 198533 Opened 21 years ago Closed 20 years ago

Make nsXULElement extend nsGenericElement

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: john, Assigned: smaug)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 11 obsolete files)

IRC log, with gory details
5.51 KB, text/plain
Details
alternate patch, work in progress
112.40 KB, patch
Details | Diff | Splinter Review
v 1.3 + comments from sicking, bz, bryner, peterv and jst
95.77 KB, patch
Details | Diff | Splinter Review 
nsXULElement duplicates sufficient functionality from nsGenericElement that one
should really inherit the other.
See bug 197427.  It's definitely related.
Blocks: 197427
nsXULElement contains mChildren, and we want mChildren to hold attributes too.
However xul-elements need to query the prototype for attributes so that we won't
have to copy them all over when the element is set up. We could possibly do this
by sticking a flag in the xulelement domslots indicating if the attributes in
mChildren are "valid" or not.
We just had a nice discussion on IRC about this.  The following suggestions were
aired (or thought of by me as I typed this):

1)  Eliminate nsGenericContainerElement and merge its functionality into
    nsGenericElement.  This means everything would have the capability to have
    children.  We need this anyway, since in an XHTML document a <link> should
    be able to have kids per hixie and peterv.
2)  Combine mChildren and mAttributes into a single array to prevent #1 from
    being bloat-city (jst has a thought or two on this, apparently).
3)  Do something about the fact that nsGenericElement has DOMSlots and nodeinfo,
    which nsXULElement does not want (prototypes for all elements?)
4)  Eliminate nsXMLElement or merge it into nsGenericElement or something (eg
    that would give XHTML and XUL elements xlink support).
Blocks: 226620
Bz had a very neat idea that might simplify this bug a lot, just wanted to get
it written down so that it doesn't get forgotten.

One of the bigger problems with having nsXULElement inheriting nsGenericElement
is that nsGenericElement has a bigger slots structure. We could have a
base-slots-struct called something like genericslots (maybe scoped in
nsGenericElement as nsGenericElement::Slots). We could then inherit that struct
and add needed members for the different subclasses, such as
nsGenericContainerElement and nsXULElement.

We'd then have a virtual 'EnsureSlots' that is overridden so that we always
create the right type of slots.

Whatever members doesn't exist in both the xulslots and the domslots will have
live in the separate subclassed slots-structures. This will unfortunatly also
force the code using those members to be move to respective subclass.

Hopefully over time we will be able to make the different slots structures more
and more similar, which means that we can move more and more code the the
baseclass. But I think that can happen over time. Especially considering that
the more we merge the cleaner stuff will be and it'll be easier to merge even more.


Another thing that is woth considering is moving members out of the slots and
into hashtables either per-document ones, or global ones. This can both reduce
the differences between the slots, and reduce the number of slots getting
created, meaning we'll have to worry less about adding other members to the
slots. (immidate canditate for such a move is mAttributeMap)
Blocks: 121495
Just found a scary function: nsXULDocument::CreateOverlayElement. Just from
reading the comment it seems to be doing something evil that requires ranges in
XUL not to work. Not sure if it's really something to worry about, but worth
keeping an eye on.
That comment sounds to me more like DOM ranges broke if a nsXMLElement was used
there...  Frankly, I would try using nsXMLElement there and testing ranges,
first thing.  ;)
We talked a bit on irc about how much bloat it would add to move mNodeInfo from
slots to nsXULElement.

Bringing up the browser window creates 1557 xul-elements based on prototypes and
23 based on nodeinfos. 286 elements were deleted bringing the total of elements
alive to 1294. A total of 40 slots were still alive.

This means that moving mNodeInfo from the slots to the element would:

Add (1294 - 40) * 4 bytes from more mNodeInfo pointers being allocated.
Save 23 * 3 * 4 bytes from saved slots not being created from nodeinfo based
elements. (actually, this might not be true since some of these elements could
have been deleted, and some might have gotten slots anyway)

Giving a total of 4740 bytes added.


Surfing to a few websites and bringing down some menues gives:
2039 proto-based elements, 44 nodeinfo based, 337 deleted (total 1746) and 56
alive slots.

This gives 6232 bytes added.

This should probably be multiplied a bit given that you often run with several
windows open.

Is this something we could live with?


Another interesting observation here is that we create incredibly few slots
these days (this probably decreased a lot with bug 195350 since we no longer
allocate slots when local attributes are set). This means that we can probably
without worry use the nsDOMSlots from nsGenericElement without worrying about
added bloat.
Depends on: 235640
Seems fairly reasonable to me. Were those numbers from SeaMonkey, or FireFox?
What about Mail/Thunderbird? I don't expect any surprises here, but verifying
that no apps generate an order of magnitude more proto based XUL elements would
be a good thing to do... And how do multiple windows affect this, does it scale
up linearly?
The numbers are seamonkey. I didn't test mail or firefox/thunderpants.
Opening new windows scale pretty lineraly, but opening new tabs adds very few
elements.
This is great work. On behalf of caring technical users,
though, we're still anxious about bloat. Its Moz v IE v Safari
v Opera, not just Moz v Moz.

We love that we get .style and printing in XUL, because it lets
us re-use headspace between XUL and HTML. Non-visible, internal
re-design at the cost of bloat, however, isn't a headspace saving,
at least not for XML heads. Some questions:

1. Is there a performance/startup benefit to this code?
   Co-location of text might reduce the active page set or page faults?
2. Would this change enable opportunities for future decreases
   elsewhere, or is it a one-time hit?
3. Is there a good bloat saving from reduced text?

Guesswork on 3. says:

[nrm@saturn src]$ size nsXULElement.o
   text    data     bss     dec     hex filename
 140432    4096       0  144528   23490 nsXULElement.o

[nrm@saturn src]$ size nsGenericElement.o
   text    data     bss     dec     hex filename
 101784    3328       0  105112   19a98 nsGenericElement.o

Looks like there's a lot of text to potentially reduce,
but this is just a litmus test, not a proper objdump(1) analysis.

A back-of-the-envelope objdump(1) analysis says *something like*
46500 (nsXULElement) - 23604 (nsGenericElement) = 22896 bytes
of text maximally saveable before new code is added and before
Life intrudes. Estimates based on unoptimised code.

Keen to vote for this bug if any external benefits poke out.
But we hate bloat. Hope this feedback helps.

- N.
Blocks: 55801
Blocks: 69996
First off, if the added size is only 6k per browserwindow noone is going to
notice a difference, I promice. In fact, the sheer codesize savings are probably
going to outweigh it in the common case.

> 2. Would this change enable opportunities for future decreases
>    elsewhere, or is it a one-time hit?

Yes it would. The current design leads to codecomplexity making codeimprovements
hard and maintainece a lot of work.

On another note, peterv figured out a way to get mDocument out from mNodeInfo,
so having a proper mNodeInfo won't actually add any bloat.


fwiw, i've done 1) and 2) in the list in comment 3. So 3) would be next :)
I can't wait for XUL elements to get a proper native anonymous flag
implementation - the current one is almost as buggy as the previous one.
Attached patch v 1 (obsolete) — — Splinter Review 
Initial patch to make nsXULElement to inherit nsGenericElement.
This splits nsDOMSlots to base class nsDOMSlots and subclasses nsXMLDOMSlots
(non-xul-elements) and nsXULDOMSlots.
Removes nsIDOMNode/Element duplicates. nsIContent methods are removed only
they are trivial or otherwise needed changes because of the new inheritance.
Comments?
Attached patch v 1.0.1 (obsolete) — — Splinter Review 
oops, this one.
Attachment #169128 - Attachment is obsolete: true
uhm, did you read the last paragraph of comment 8? I don't think we have to
worry very much about the size of the slots structure.
I wasn't worrying about the size of slots too much, but nsGenericElement and 
nsXULElement just need a bit different kind of slot structure.
That was the reason to create nsXMLDOMSlots and nsXULDOMSlots.
Can't we use one DOMSlots structure?

DOMSlots:

mFlags
mChildNodes
mStyle
mAttributeMap
mBindingParent
mContentID

nsXULElement::Slots:

mControllers
mDOMStyle
mAttributeMap
mChildNodes
mLazyState
mHasProperties

Unifying:

mFlags
mChildNodes
mStyle
mAttributeMap
mBindingParent
mContentID/mControllers

mLazyState and mHasProperties can go into mFlags. I'm assuming we don't want to
start adding content IDs to XUL elements. You'd have to check whether the fact
that mBindingParent is in the slots doesn't cause us to create too many slots.
If it does we can always keep the second mBindingParent in nsXULElement. Thoughts?

Yeah, that's what I had in mind too. And from XULs point of view I don't even
think we need to combine mControllers and mContentID since we during an average
scenario create well below 100 slots. Could be that we create more for HTML though.

I think we'll have to keep mBiningParent in nsXULElement (though commented
thorouly both in nsDOMSlots and nsXULElement to avoid confusion).
Starting up the browser creates ~190 (xul)slots (with or without the patch).
Most of the slots have their mChildNodes != nsnull.
Binding parent is set to ~275 xul elements, and usually those elements don't
have slots. 
1589 xul elements are created, 262 deleted.

I don't know why I see so much more slots created than Jonas did.
(I added a counter to the ctor of the slot)

I'd keep the mBindingParent in the XULElement.
And actually I still don't quite understand why not to use 
nsDOMSlots/nsXMLDOMSlots/nsXULDOMSlots. For me it seems somehow clearer way to
do this.
But it is also ok to use mBindingParent as mLazystate and mContentID as
mController.

I just think the code would be simpler and have less virtualness with a single
slots class. I'm a strong KISS supporter (and not just the rock-group) :)

It is strange that you got so many more ctors then me. I don't remember exactly
how I counted them, but the comment seems to indicate that I counted created
minus destroyed and once the browser was loaded that was 40. I do remember a lot
of xul elements being created and then destroyed. It could also simply be that
it has changed since I ran the tests.
Attached patch v 1.1 (obsolete) — — Splinter Review 
Using unions in nsDOMSlots?
Attachment #169133 - Attachment is obsolete: true
This looks great. A few of random comments from skimming through the patch, no
need to create a new patch just yet:

First off, this needs a lot of testing. I havn't looked yet at how much the
behaviour of nsXULElement has changed, but there is a fair amount of code in the
tree that relies on various (sometime very subtle) bugs in nsXULElement. While
it would be nce to get rid of these bugs that is probably better delt with
separatly. Now is a good time for this work though since we're in alpha.

Could mLazyState be part of mFlags instead as peterv suggested? I think there
should be plenty of bits still available there (i.e, don't use a union, just use
some of the available bits). While we're on the topic, i wonder if mFlags should
use bitfields instead of macros. That could wait though.

In nsXULElement::CloneNode there is no need to ASSERT or ENSURE that slots exist
since all you want is the nodeinfo that doesn't live in slots any more. There
might be other places like this too.

in nsXULElement::ClearLazyState you call HasDOMSlots and then GetDOMSlots. It'd
be more efficient to call GetExistingDOMSlots. Again, there might be other
places of this...

In cases where you know that the element is in the document call GetOwnerDoc()
rather then GetCurrentDoc(), that's faster iirc.

I saw some places where you changed NodeInfo() to GetNodeInfo(), could you
change that to mNodeInfo instead for speed? (GetNodeInfo is virtual)
Thanks for comments. I was just going through the changes to optimize
and clean up things.
I didn't want to use mFlags for lazy state, because it would
mix a bit the genericelement implementation and xul element implementation.
I tried to keep those things in the domslots...and there was space anyway
for lazystate so... :)

This patch shouldn't change the behaviour of the XUL elements.
Only those DOM methods, which are pretty much exact duplicates were
removed. For example GetAttribute is still there because the return value
is not the same as in GenericElement.
But testing is needed!

well, this bug is about merging xulelement and genericelement so i don't see a
reason to keep them apart in domslots :)

I'm reassigning to you, thanks for the excellent work!

Once this bug is checked in there is probably room for some cleanup in
nsGenericElement since some of the methods there are designed so that they can
be called from nsXULElement. But lets do that separatly.
Assignee: general → smaug
(In reply to comment #25)
>For example GetAttribute is still there because the return value
>is not the same as in GenericElement.
A list of these differences would be nice, so we can fix the code to expect
generic return values instead of XUL-specific hacks.
once this bug is fixed nsXULElement.cpp itself should be quite a nice list :)

The GetAttribute difference is tracked in bug 232598 btw.
Attached patch v 1.2 (obsolete) — — Splinter Review 
This is using the flags also for lazy state.
Maybe Jonas has time to look at this?
Attachment #169239 - Attachment is obsolete: true
Attachment #169317 - Flags: review?(bugmail)
I won't have time to review this before i get back from vacation 12/31
Comment on attachment 169317 [details] [diff] [review]
v 1.2

>Index: content/xul/content/src/nsXULElement.cpp

> nsXULElement::~nsXULElement()
> {
>+    //XXX SetDocument(nsnull) is not called always before dtor.
>+    //XXX Related to templates or overlays?
>+    SetDocument(nsnull, PR_TRUE, PR_FALSE);
>+
>+    nsDOMSlots* slots = GetExistingDOMSlots();
>+    if (slots) {
>+      NS_IF_RELEASE(slots->mControllers); // Forces release
>+      slots->mControllers = nsnull;

No need to set to nsnull, the macro does that.


>@@ -487,25 +433,47 @@ nsXULElement::Create(nsXULPrototypeEleme
>     NS_PRECONDITION(aPrototype != nsnull, "null ptr");
>     if (! aPrototype)
>         return NS_ERROR_NULL_POINTER;
> 
>     NS_PRECONDITION(aResult != nsnull, "null ptr");
>     if (! aResult)
>         return NS_ERROR_NULL_POINTER;
> 
>-    nsRefPtr<nsXULElement> element = new nsXULElement();
>+    nsCOMPtr<nsINodeInfo> nodeInfo;
>+    nsresult rv;
>+    if (aDocument &&
>+        aDocument != aPrototype->mNodeInfo->NodeInfoManager()->GetDocument()) {

The second check should be unneccesary, prototypes always use 'anonymous'
nodeinfos.

>+        nsCOMPtr<nsINodeInfo> ni = aPrototype->mNodeInfo;
>+        if (!ni)
>+            return NS_ERROR_NULL_POINTER;

No need to do this, all prototypes have nodeinfos, we would have crashed a long
time ago otherwise.

...
>     element->mPrototype = aPrototype;
>-    element->mDocument = aDocument;
>+    if (aDocument) {
>+      element->mParentPtrBits |= PARENT_BIT_INDOCUMENT;
>+    }
>+    else {
>+      element->mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
>+    }

I wonder if it'd be better to call SetDocument here?

>@@ -884,29 +573,26 @@ NS_IMETHODIMP
> nsXULElement::CloneNode(PRBool aDeep, nsIDOMNode** aReturn)
> {
>     nsresult rv;
> 
>     nsCOMPtr<nsIContent> result;
> 
>     // If we have a prototype, so will our clone.
>     if (mPrototype) {
>-        rv = nsXULElement::Create(mPrototype, mDocument, PR_TRUE,
>+        rv = nsXULElement::Create(mPrototype, GetCurrentDoc(), PR_TRUE,
>                                   getter_AddRefs(result));

Hmm.. tricky. This won't work when calling cloneNode on an element not in the
document. You'll call nsXULElement::Create with a null document which means
that the newly created element will get its nodeinfo from the prototype, this
nodeinfo doesn't have a document.

The best solution would probably be to add another argument to
nsXULElement::Create, |nsIDocument* aOwnerDocument| or something. Please check
all callers of Create to see if a similar problem exists elsewhere.


> nsXULElement::SetDocument(nsIDocument* aDocument, PRBool aDeep,
...
>         // mControllers can own objects that are implemented
>         // in JavaScript (such as some implementations of
>         // nsIControllers.  These objects prevent their global
>         // object's script object from being garbage collected,
>         // which means JS continues to hold an owning reference
>         // to the nsGlobalWindow, which owns the document,
>         // which owns this content.  That's a cycle, so we break
>-        // it here.  (It might be better to break this by releasing
>-        // mDocument in nsGlobalWindow::SetDocShell, but I'm not
>-        // sure whether that would fix all possible cycles through
>-        // mControllers.)

Why remove this comment?

>-        if (!aDocument && mSlots) {
>-            mSlots->mControllers = nsnull; // Forces release
>+        // it here.
>+        if (!aDocument && HasDOMSlots()) {
>+            nsDOMSlots* slots = GetExistingDOMSlots();

It'd be faster to move the GetExistingDOMSlots call to outside the |if| and use
slots in the condition.

>+            NS_IF_RELEASE(slots->mControllers); // Forces release
>+            slots->mControllers = nsnull;

no need to set to null here.


>@@ -1937,17 +1294,17 @@ nsXULElement::RemoveChildAt(PRUint32 aIn
>             if (xulCurItem)
>                 controlElement->SetCurrentItem(xulCurItem);
>         } else {
>             controlElement->SetCurrentItem(nsnull);
>         }
>     }
> 
>     if (fireSelectionHandler) {
>-      nsCOMPtr<nsIDOMDocumentEvent> doc(do_QueryInterface(mDocument));
>+      nsCOMPtr<nsIDOMDocumentEvent> doc(do_QueryInterface(GetCurrentDoc()));
>       nsCOMPtr<nsIDOMEvent> event;
>       doc->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));

Make that |doc|, though you'll have to name that comptr something else.

Unrelated to this patch I'm a little scared that we'll return null which will
crash. Feel free to add |&& doc| to that if-statment.


more to come...
Comment on attachment 169317 [details] [diff] [review]
v 1.2

I just noticed that you had to make nsIXULContent not inherit nsIXMLContent.
I'm surpriced that didn't force you to add a bunch of extra QIs. Or did you
have to and I just missed them?


In nsXULElement::SetDocument
>+        if (IsInDoc()) {
>+            // get a new nodeinfo
>+            nsNodeInfoManager* nodeInfoManager = aDocument->NodeInfoManager();
>+            if (nodeInfoManager) {
>+              nsCOMPtr<nsINodeInfo> newNodeInfo;
>+              nodeInfoManager->GetNodeInfo(mNodeInfo->NameAtom(),
>+                                           mNodeInfo->GetPrefixAtom(),
>+                                           mNodeInfo->NamespaceID(),
>+                                           getter_AddRefs(newNodeInfo));
>+              if (newNodeInfo) {
>+                mNodeInfo.swap(newNodeInfo);
>+              }
>+            }

Here you should check that the nodeinfo doesn't already belong to the right
document (we might be, and probably are, reinserting the element into the
document where it was before).

> nsXULElement::List(FILE* out, PRInt32 aIndent) const
> {
>-    NS_PRECONDITION(mDocument != nsnull, "bad content");
>+    NS_PRECONDITION(GetCurrentDoc() != nsnull, "bad content");

Not an important function I know... But make that |IsInDoc()|

>@@ -2636,17 +1983,17 @@ nsXULElement::HandleDOMEvent(nsPresConte
>     nsIDOMEvent* domEvent = nsnull;
>     if (NS_EVENT_FLAG_INIT & aFlags) {
>         if (aEvent->message == NS_XUL_COMMAND && Tag() != nsXULAtoms::command) {
>             // See if we have a command elt.  If so, we execute on the command instead
>             // of on our content element.
>             nsAutoString command;
>             GetAttr(kNameSpaceID_None, nsXULAtoms::command, command);
>             if (!command.IsEmpty()) {
>-                nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(mDocument));
>+                nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(GetCurrentDoc()));
>                 nsCOMPtr<nsIDOMElement> commandElt;
>                 domDoc->GetElementById(command, getter_AddRefs(commandElt));

I guess we shouldn't reach this if we're not in the document, so we shouldn't
crash. But make that GetOwnerDoc() just in case. (It also happens to be
smaller.)

> nsXULElement::EnsureContentsGenerated(void) const
> {
>-    if (mSlots && (mSlots->mLazyState & nsIXULContent::eChildrenMustBeRebuilt)) {
>+    if (GetFlags() & GENERIC_ELEMENT_CHILDREN_TO_REBUILT) {
>         // Ensure that the element is actually _in_ the document tree;
>         // otherwise, somebody is trying to generate children for a node
>         // that's not currently in the content model.
>-        NS_PRECONDITION(mDocument != nsnull, "element not in tree");
>-        if (!mDocument)
>+        NS_PRECONDITION(GetCurrentDoc() != nsnull, "element not in tree");
>+        if (!IsInDoc())
>             return NS_ERROR_NOT_INITIALIZED;

Use IsInDoc in the precondition too.

> nsXULElement::GetControllers(nsIControllers** aResult)
> {
>     if (! Controllers()) {
>-        NS_PRECONDITION(mDocument != nsnull, "no document");
>-        if (! mDocument)
>+        NS_PRECONDITION(GetCurrentDoc() != nsnull, "no document");
>+        if (!IsInDoc())
>             return NS_ERROR_NOT_INITIALIZED;

Here too.

>+        if (!GetDOMSlots())
>+          return NS_ERROR_OUT_OF_MEMORY;
>+
>         nsresult rv;
>-        rv = EnsureSlots();
>-        if (NS_FAILED(rv)) return rv;
>+        nsCOMPtr<nsIControllers> controllers;
>+        rv = NS_NewXULControllers(nsnull, NS_GET_IID(nsIControllers),
>+                                  getter_AddRefs(controllers));
> 
>-        rv = NS_NewXULControllers(nsnull, NS_GET_IID(nsIControllers), getter_AddRefs(mSlots->mControllers));
>         NS_ASSERTION(NS_SUCCEEDED(rv), "unable to create a controllers");
>         if (NS_FAILED(rv)) return rv;
> 
>+        NS_ADDREF(GetDOMSlots()->mControllers = controllers);

Store the value of the first GetDOMSlots call so that you don't have to call it
twice. And why not use |&slots->mControllers as argument to
NS_NewXULControllers directly?

> nsXULElement::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)
> {
>-    nsresult rv = EnsureSlots();
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsDOMSlots* slots = GetDOMSlots();
>+    if (!slots)
>+        return NS_ERROR_OUT_OF_MEMORY;

You could use NS_ENSURE_TRUE(slots, NS_ERROR_OUT_OF_MEMORY) here.

> nsresult nsXULElement::MakeHeavyweight()
> {
>-    NS_ASSERTION(mPrototype || (mSlots && mSlots->mNodeInfo), "need prototype or nodeinfo");
>+    NS_ASSERTION(mPrototype || (mNodeInfo), "need prototype or nodeinfo");

You could remove this assertion now. We should always have an mNodeInfo.

>     if (!mPrototype)
>         return NS_OK;           // already heavyweight
> 
>-    nsresult rv = EnsureSlots();
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (!GetDOMSlots())
>+      return NS_ERROR_OUT_OF_MEMORY;

No need to generate slots at all any more. The only reason it called
EnsureSlots was to make sure mSlots->mNodeInfo got set, which of course isn't
needed any more.

> nsXULElement::HideWindowChrome(PRBool aShouldHide)
> {
>-    nsIPresShell *shell = mDocument->GetShellAt(0);
>+    if (!IsInDoc())
>+      return NS_OK;
>+    nsIPresShell *shell = GetOwnerDoc()->GetShellAt(0);

Make this return NS_ERROR_UNEXPECTED or something. If we crashed on it before I
doubt it's an ok situation. And please insert an empty line after the return
for readability.


>Index: content/xul/content/src/nsXULElement.h

>-    nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>-                     const nsAString& aValue, PRBool aNotify)
>-    {
>-      return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify);
>-    }

You'll still need this. Otherwise you'll get warnings and errors about hidden
functions. I'm surpriced it actually builds without it...

That's it! Awesome patch! Fix these things and I'll r+ it.
Attachment #169317 - Flags: review?(bugmail) → review-
> In cases where you know that the element is in the document call GetOwnerDoc()
> rather then GetCurrentDoc(), that's faster iirc.

Actually, it's not.  Worse yet, the two methods may return _different_ pointers for nodes 
that are in a document if we implement sXBL or XBL2.  So please call whichever one 
makes sense in the context in which you need the document (that is, do you need the 
document the node is being rendered in, or the document it "comes from"?).
And if you can't decide whether to use GetOwnerDoc() or GetDocument() (like in the XUL 
command thing, because it's underdefined), add a comment mentioning "sXBL/XBL2", 
like the one at http://lxr.mozilla.org/seamonkey/source/content/html/content/src/
nsGenericHTMLElement.cpp#3296 or something.
Erm, mind explaining that a bit more? When could the two differ and what value
would the two return?

Seems like that is a bigger change then should be tackled in this bug.
Especially since this bug is a big change on its own.
> Erm, mind explaining that a bit more? When could the two differ and what value
> would the two return?

For anonymous content, the owner doc is the xbl document and the current doc is the 
current doc of the binding parent.

> Seems like that is a bigger change then should be tackled in this bug.

Sure.  I'm not suggesting we change anything.  I'm suggesting that any new callers of 
GetOwnerDoc/GetCurrentDoc that we add call the right function.  The two return 
conceptually different things, and should not be treated as synonyms for each other.
Well, then we need some function that doesn't check IsInDoc() before going to
the nodeinfo to look for the document. Or do you propose that GetCurrentDoc()
will be that function?

IMHO lets do what's best with the current code and worry about this stuff once
we know that what sXBL/XBL2 looks like and how we want to implement it.
All I am proposing is that callers call the appropriate function -- the one that corresponds 
to the document they want.  The actual implementation should be of sublime disinterest 
to the callers, which will allow us to meddle with the implementation as desired if or when 
we implement sXBL/XBL2.

So all I am asking for is that we not tweak which of the two functions we are calling now 
based on criteria other than "which document do I want?"

I'd really like us to get this right up front, because auditing all callers of the two methods 
later will be near-impossible. I did that for the existing GetOwnerDoc/GetCurrentDoc 
callers recently, and it took me over a week.  I estimate that less than 10% of 
GetDocument() callers had been converted at that point....
Ok, sounds resonable. So in what instances do we want the ownerDocument? Seems
like for almost everything we're insterested in GetCurrentDoc?
(In reply to comment #31)
> >-    element->mDocument = aDocument;
> >+    if (aDocument) {
> >+      element->mParentPtrBits |= PARENT_BIT_INDOCUMENT;
> >+    }
> >+    else {
> >+      element->mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
> >+    }
> 
> I wonder if it'd be better to call SetDocument here?

That would change the way XUL element works atm. I'd like
to keep the functionality as much as possible the same it is now.

> 
> >@@ -884,29 +573,26 @@ NS_IMETHODIMP
> > nsXULElement::CloneNode(PRBool aDeep, nsIDOMNode** aReturn)
> > {
> >     nsresult rv;
> > 
> >     nsCOMPtr<nsIContent> result;
> > 
> >     // If we have a prototype, so will our clone.
> >     if (mPrototype) {
> >-        rv = nsXULElement::Create(mPrototype, mDocument, PR_TRUE,
> >+        rv = nsXULElement::Create(mPrototype, GetCurrentDoc(), PR_TRUE,
> >                                   getter_AddRefs(result));
> 
> Hmm.. tricky. This won't work when calling cloneNode on an element not in the
> document. You'll call nsXULElement::Create with a null document which means
> that the newly created element will get its nodeinfo from the prototype, this
> nodeinfo doesn't have a document.
> 
> The best solution would probably be to add another argument to
> nsXULElement::Create, |nsIDocument* aOwnerDocument| or something. Please check
> all callers of Create to see if a similar problem exists elsewhere.
> 

This is difficult. XUL/XBL is broken here. See the comment (from CloneNode()):
         // 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);
+        result->SetDocument(GetCurrentDoc(), PR_TRUE, PR_TRUE);

Would it make sense to have
nsIDocument* doc = IsInDoc() ? GetCurrentDoc() : GetOwnerDoc();
and then use 'doc' as the document? That should make sure that
we get a document.
Status: NEW → ASSIGNED
ok, here's what I think should work:

always pass GetOwnerDoc() to ::Create since you want the nodeinfo to come from
the owner document. Then right after, add

if (!IsInDoc()) {
    mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
}

That should create the same behaviour as before, right?
(In reply to comment #39)
> Ok, sounds resonable. So in what instances do we want the ownerDocument? Seems
> like for almost everything we're insterested in GetCurrentDoc?

The general rule of thumb is that you want the ownerDocument for things like security 
checks, URI resolution, script contexts, DOM stuff, etc, etc.  You want the currentDoc for 
things having to do with rendering (getting presshells, basically, but some types of event 
propagation may fall in there; this part is not quite clear to me and should just get 
comments as I requested).
Attached patch v 1.3 (obsolete) — — Splinter Review 
Changed some GetOwnerDoc to GetCurrentDoc, added mParentPtrBits &=
~PARENT_BIT_INDOCUMENT to CloneNode.
Attachment #169317 - Attachment is obsolete: true
Attachment #169429 - Flags: review?(bugmail)
Comment on attachment 169429 [details] [diff] [review]
v 1.3

>     if (mPrototype) {
>-        rv = nsXULElement::Create(mPrototype, mDocument, PR_TRUE,
>+        rv = nsXULElement::Create(mPrototype, GetOwnerDoc(), PR_TRUE,
>                                   getter_AddRefs(result));
>         NS_ENSURE_SUCCESS(rv, rv);
>-    } else {
>-        NS_ASSERTION(mSlots, "no prototype and no slots!");
>-        NS_ENSURE_TRUE(mSlots, NS_ERROR_UNEXPECTED);
> 
>-        rv = NS_NewXULElement(getter_AddRefs(result), mSlots->mNodeInfo);
>+        if (!IsInDoc()) {
>+            nsIContent* element = result;
>+            nsXULElement* xulElement = NS_STATIC_CAST(nsXULElement*, element);
>+            xulElement->mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;

Ugh, that's evil. But I can't think of another way to do it. No need to create
two temporaries though. Either eliminate |element| by casting twice, or
elminiate xulElement by doing

NS_STATIC_CAST(nsXULElement*, element)->mParentPtrBits &=
~PARENT_BIT_INDOCUMENT;


> nsXULElement::SetDocument(nsIDocument* aDocument, PRBool aDeep,
...
>+        if (IsInDoc()) {
>+            if (aDocument != mNodeInfo->NodeInfoManager()->GetDocument()) {
>+              // get a new nodeinfo

You could compare 'aDocument != doc' here. Though it probably won't be right in
the sXBL/XBL2 world, but I would think that this entire function needs
rewriting then. So either do that and add the comment Boris requested, or
change it to
aDocument != mNodeInfo->GetDocument()


You're still creating that temporary nsCOMPtr in nsXULElement::GetControllers,
just pass |&slots->mControllers| directly.


The assertion and the slots-creating is still there in MakeHeavyWeight. Just
remove them.


r=sicking with that.
Attachment #169429 - Flags: review?(bugmail) → review+
> 
> You're still creating that temporary nsCOMPtr in nsXULElement::GetControllers,
> just pass |&slots->mControllers| directly.
> 

Compiler didn't really like that :(




Bah, that is one crappy signature. You'll have to do

NS_REINTERPRET_CAST(void**, &slots->mControllers)
Attached patch v 1.3 + sicking's comments (obsolete) — — Splinter Review 

        Attachment #169429 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v 1.3 + sicking's comments (obsolete)
        — 
          — Splinter Review
      

    


  
sorry, this one.

        Attachment #169435 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 169436 [details] [diff] [review]
v 1.3 + sicking's comments

wow, diff jacked up this patch bad. I'd recommend whoever's superreviewing this
to look at the 1.3 version and then use the bugzilla interdiff tool to see the
last changes.

Anyway, look good, r=me

        Attachment #169436 -
        Flags: review+

    
    

    
  
Drive-by comments:

Was there a reason to leave the broken nsXULElement::GetBaseURI instead of using the 
nsGenericElement one like the XXX comment says?

nsXULElement::AddScriptEventListener and nsXULElement::CompileEventHandler 
probably want the owner doc, not the current one (and an XXX comment about sorting it 
out for sure, perhaps).

The code that gets the binding manager in nsXULElement::List should also have such a 
comment.  Same for the code doing command lookups in HandleDOMEvent and getting 
the binding manager there.  Same for GetBuilder.  Same for GetControllers.   Same for 
GetBoxObject.  

    
    

    
  


  
using nsGenericElement::GetbaseURI, changed the document in 
nsXULElement::AddScriptEventListener and nsXULElement::CompileEventHandler, 
added comments.

        Attachment #169436 -
        Attachment is obsolete: true

        Attachment #169655 -
        Flags: superreview?(jst)
Blocks: 275159
Flags: blocking1.8a6?

    
    

    
  


  
I've been out on vacation and just now ran across this bug.  Unfortunately, I'd
decided to work on this same thing during my time off, and didn't know anyone
else was working on it.  I hadn't planned on posting this until I'd had a
chance to clean it up some more, however, I want to get this out there since I
did do some things rather differently.

Basically, I had a few extra goals in mind:
- I wanted to not add any extra bloat to nsXULElement.	I think this can be
accomplished by making the prototype and nodeinfo a union, and doing that
required factoring out some base parts of the prototype mechanism to
nsGenericElement.
- As a result of the above, all element classes can share the same nodeinfo
storage.  Once that's the case, I'm thinking we could move element-related
methods such as Tag(), GetNameSpaceID(), GetNodeInfo(), GetChildCount(), etc,
as well as the data members, onto a new interface, nsIElement.	That would
enable many of these methods to have trivial inline implementations.

I also did away with nsIXULContent, instead just allowing a cast based on the
namespace (this will need to be #ifdef'd for --disable-xul).  This interface is
not used at all from outside of gklayout.

    
    

    
  
(In reply to comment #52)
> - I wanted to not add any extra bloat to nsXULElement. I think this can be
> accomplished by making the prototype and nodeinfo a union, and doing that
> required factoring out some base parts of the prototype mechanism to
> nsGenericElement.

I don't think there's any need for that union, the addition of mNodeInfo is
compensated by the removal of mDocument (see attachment 169655 [details] [diff] [review]).

    
    

    
  
(In reply to comment #53)
> (In reply to comment #52)
> > - I wanted to not add any extra bloat to nsXULElement. I think this can be
> > accomplished by making the prototype and nodeinfo a union, and doing that
> > required factoring out some base parts of the prototype mechanism to
> > nsGenericElement.
> 
> I don't think there's any need for that union, the addition of mNodeInfo is
> compensated by the removal of mDocument (see attachment 169655 [details] [diff] [review] [edit]).

Ah, I didn't notice the removal of mDocument (though I think I did comment on
the possibility of removing it in my patch).  At any rate, the nodeinfo and
prototype are indeed redundant since the nodeinfo can be obtained from the
prototype if one is in use.  Doing that _and_ removing mDocument would actually
reduce the size of XULElements. (There is an edge case where we need a new
nodeinfo because the DOMNode setPrefix() is called, and with this change that
causes the element to detach from the prototype, but I don't consider this case
worth optimizing).


    
    

    
  
Unfortunatly the nodeinfo in the prototype isn't good enough. The problem is
that that it's impossible to get back to the originating document from it which
is causing some random problems such as bug 235640. This also makes it
impossible to remove mDocument. Note that the same prototype is used for
elements in several different documents.

    
    

    
  
Getting rid of nsIXULElement and having an nsIElement sounds like a good idea
though. But maybe we could do that separatly from smaugs patch?

    
    

    
  
(In reply to comment #55)
> Unfortunatly the nodeinfo in the prototype isn't good enough. The problem is
> that that it's impossible to get back to the originating document from it which
> is causing some random problems such as bug 235640. This also makes it
> impossible to remove mDocument. Note that the same prototype is used for
> elements in several different documents.

Ah, right.  So then the only question is whether calling
nsNodeInfoManager::GetNodeInfo() when constructing an element with a prototype
will adversely affect new window time.  I did notice this:

+    if (aDocument) {
+        nsCOMPtr<nsINodeInfo> ni = aPrototype->mNodeInfo;
+        rv = aDocument->NodeInfoManager()->GetNodeInfo(ni->NameAtom(),
+                                                       ni->GetPrefixAtom(),
+                                                       ni->NamespaceID(),
+                                                       getter_AddRefs(nodeInfo));
         NS_ENSURE_SUCCESS(rv, rv);
     }

No need for |ni| to be a nsCOMPtr.  Someone should run through the Txul test
before checking this in.

I think with this approach I can still do everything I wanted to do on
nsIElement, the only requirement was that accessing the nodeinfo work in the
same way for all elements.


    
    

    
  
also,

+#define GENERIC_ELEMENT_CHILDREN_TO_REBUILT    0x00000020U

This #define name doesn't really make sense to me... I'd suggest
GENERIC_ELEMENT_CHILDREN_TO_REBUILD or maybe GENERIC_ELEMENT_CHILDREN_TO_BE_REBUILT.


    
    

    
  
I'd think that the extra hash-lookup should be fine (especially given the fast
hashfunction), though testing is defenetly a good idea.

I filed bug 276698 on nsIElement

    
    

    
  
Oh, if someone runs Txul tests on this patch and we end up being slower, it
might be worth trying to put back the nsXULElement::GetBaseURI implementation.
I'm not sure how much that function is called, but the version in
nsGenericElement is slower for sure. If this turns out to 'fix' the problem we
should leave the GetBaseURI implementation in and tackle it in a separate bug.

    
    

    
  


  
tested using CYCLES == 10
####################without patch
__xulWinOpenTime:306
__xulWinOpenTime:308
__xulWinOpenTime:310
__xulWinOpenTime:311

####################with patch
__xulWinOpenTime:307
__xulWinOpenTime:307
__xulWinOpenTime:313
__xulWinOpenTime:314

####################with patch, using original GetBaseUri
__xulWinOpenTime:306
__xulWinOpenTime:307
__xulWinOpenTime:312
__xulWinOpenTime:314

        Attachment #169655 -
        Attachment is obsolete: true

        Attachment #170072 -
        Flags: superreview?(jst)

        Attachment #169655 -
        Flags: superreview?(jst)

    
    

    
  
One way to speed the nodeinfo "translation" up would be to do something like this:

Instead of letting each prototype element hold a nodeinfo we let them hold onto
an integer index. This would be an index into an array of nodeinfos that lives
in the prototype document.
When we then create the dom-document out of a prototype tree we start by
"translating" the array creating an array with nodeinfos created by the
doc-documents nodeinfomanager. Then when creating the dom-element we just use
the index to get the nodeinfo from the "translated" array.

This should almost compleatly remove the lookup time for new nodeinfos.

However this could be more complicated then it sounds. Especially considering
overlay documents (each overlay is a separate prototype document that would need
translation) and the fact that we often create xul-documents by cloning existing
documents rather then build new ones by wrapping prototypes. Also, prototypes
don't have a pointer back to their documents (nor parents) which makes it hard
to get to the nodeinfo array from a prototype.

One thing that could simplify this might be to create a global nodeinfo array
that all prototypes use. We would then have to make sure the indexes were
correct in the prototype as we create them.

    
    

    
  
Comment on attachment 170072 [details] [diff] [review]
v 1.3 + comments from sicking, bz and bryner

>Index: content/base/src/nsGenericElement.h
>===================================================================

>@@ -83,18 +84,30 @@ typedef unsigned long PtrBits;
> #define GENERIC_ELEMENT_HAS_LISTENERMANAGER    0x00000004U
> 
> /** Whether this content is anonymous */
> #define GENERIC_ELEMENT_IS_ANONYMOUS           0x00000008U
> 
> /** Whether this content has had any properties set on it */
> #define GENERIC_ELEMENT_HAS_PROPERTIES         0x00000010U
> 
>+/**
>+ * Next three bits are used for XUL Element's lazy state.
>+ * @see nsIXULContent
>+ */
>+#define GENERIC_ELEMENT_CHILDREN_TO_REBUILD    0x00000020U
>+
>+#define GENERIC_ELEMENT_TEMPLATE_BUILT         0x00000040U
>+
>+#define GENERIC_ELEMENT_CONTAINER_BUILT        0x00000080U
>+
>+#define GENERIC_ELEMENT_LAZY_STATE_OFFSET 5
>+
> /** The number of bits to shift the bit field to get at the content ID */
>-#define GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET 5
>+#define GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET 8

XUL element's lazy state should be in nsXULElement and there's no need to take
bits from the content ID in nsGenericElement. There's generic flags (5 for
now), and then there's specific flags. For nsGenericElement's all the specific
flags are used as bits for the content ID and for nsXULElement we use 3 of them
for the lazy state. 

>Index: content/xul/content/src/nsXULElement.cpp
>===================================================================

> nsXULElement::~nsXULElement()
> {
>+    //XXX SetDocument(nsnull) is not called always before dtor.
>+    //XXX Related to templates or overlays?
>+    SetDocument(nsnull, PR_TRUE, PR_FALSE);

Need to file a bug on this.

>+    nsCOMPtr<nsINodeInfo> nodeInfo;
>     nsresult rv;

>@@ -487,827 +432,214 @@ nsXULElement::Create(nsXULPrototypeEleme

>+    if (aDocument) {
>+        nsINodeInfo* ni = aPrototype->mNodeInfo;
>+        rv = aDocument->NodeInfoManager()->GetNodeInfo(ni->NameAtom(),
>+                                                       ni->GetPrefixAtom(),
>+                                                       ni->NamespaceID(),
>+                                                       getter_AddRefs(nodeInfo));
>         NS_ENSURE_SUCCESS(rv, rv);
>     }
>+    else
>+      nodeInfo = aPrototype->mNodeInfo;

Add braces around the else block.

>+NS_INTERFACE_MAP_BEGIN(nsXULElement)
>+    NS_INTERFACE_MAP_ENTRY(nsIStyledContent)
>+    NS_INTERFACE_MAP_ENTRY(nsIContent)
>+    NS_INTERFACE_MAP_ENTRY(nsIXULContent)
>+    NS_INTERFACE_MAP_ENTRY(nsIXMLContent)
>+    NS_INTERFACE_MAP_ENTRY(nsIDOMXULElement)
>+    NS_INTERFACE_MAP_ENTRY(nsIDOMElement)
>+    NS_INTERFACE_MAP_ENTRY(nsIDOMNode)
>+    NS_INTERFACE_MAP_ENTRY(nsIScriptEventHandlerOwner)
>+    NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventReceiver,
>+                                   nsDOMEventRTTearoff::Create(this))
>+    NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventTarget,
>+                                   nsDOMEventRTTearoff::Create(this))
>+    NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOM3EventTarget,
>+                                   nsDOMEventRTTearoff::Create(this))
>+    NS_INTERFACE_MAP_ENTRY(nsIChromeEventHandler)
>+    NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOM3Node,
>+                                   new nsNode3Tearoff(this))
>+    NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(XULElement)
>+    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMXULElement)
>+    if (IsInDoc()) {
>+        return GetCurrentDoc()->GetBindingManager()->GetBindingImplementation(this,
>+                                                                              aIID,
>+                                                                              aInstancePtr);
>+    } else
>+NS_INTERFACE_MAP_END

You should forward some of these to nsGenericElement and use
PostQueryInterface.

>+nsXULElement::CloneNode(PRBool aDeep, nsIDOMNode** aReturn)
> {

>+        if (!IsInDoc()) {
>+            nsIContent* element = result;
>+            NS_STATIC_CAST(nsXULElement*, element)->
>+                mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;

Bah, this is so wrong. A cloned element shouldn't be in the document before
being inserted. I suppose this another hack to make XBL work.


> nsXULElement::SetDocument(nsIDocument* aDocument, PRBool aDeep,
>                           PRBool aCompileEventHandlers)

>-        mDocument = aDocument;
>+        if (aDocument) {
>+          mParentPtrBits |= PARENT_BIT_INDOCUMENT;
>+        }
>+        else {
>+          mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
>+        }
> 
>-        if (mDocument) {
>+        if (IsInDoc()) {

Make this if (aDocument) and move mParentPtrBits |= PARENT_BIT_INDOCUMENT; in
here (and the else clause where this block ends).

>@@ -2320,45 +1648,45 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpa

>         // Check to see if the OBSERVES attribute is being unset.  If so, we
>         // need to remove our broadcaster goop completely.
>-        if (mDocument && (aName == nsXULAtoms::observes ||
>+        if (IsInDoc() && (aName == nsXULAtoms::observes ||

I'd just use doc instead of IsInDoc

> NS_IMETHODIMP
> nsXULElement::GetBoxObject(nsIBoxObject** aResult)
> {
>   *aResult = nsnull;
> 
>-  if (!mDocument)
>+  // XXX sXBL/XBL2 issue! Owner or current document?
>+  nsIDocument* doc = GetCurrentDoc();
>+  if (!doc)
>     return NS_ERROR_FAILURE;
> 
>-  nsCOMPtr<nsIDOMNSDocument> nsDoc(do_QueryInterface(mDocument));
>+  nsCOMPtr<nsIDOMNSDocument> nsDoc(do_QueryInterface(doc));

Just do

  nsCOMPtr<nsIDOMNSDocument> nsDoc(do_QueryInterface(GetCurrentDoc()));
  NS_ENSURE_TRUE(nsDoc, NS_ERROR_FAILURE);

    
    

    
  
Still some tests:
I modified nsNodeInfo to use nsFixedSizeAllocator, this way txul
was a bit better. (with the patch ~1000 NIs are created and of those ~350
are deleted during startup)
__xulWinOpenTime:306
__xulWinOpenTime:306
__xulWinOpenTime:307
__xulWinOpenTime:308
The differences between tests can be still just noise, I think.

        Attachment #170072 -
        Flags: superreview?(jst)

    
    

    
  


  
CloneNode problem is still there, of course.
I hope lazy state bits are now in the place where peterv wants them to be.
Btw, who might have time to sr? ;)

        Attachment #170072 -
        Attachment is obsolete: true

    
    

    
  
This way of doing the lazystat bits doesn't work. If you first set lazystate
bits and then do something that triggers the nsDOMSlots to be created the
nsDOMSlots ctor will not copy the lazystate bits into mFlags but rather set them
in mContentID.

I liked the old way of doing the lazybits better, even though some xul-specific
macros ended up in nsGenericElement.h. Loosing 3 bits of contentID gives a
maximum of 16 million, which is far more elements then we can support anyway.
And that's just before we start creating nsDOMSlots, we always support full 32
bits contentIDs, with or without this patch by placing too large ids in nsDOMSlots.

What I am a bit more concerned about though is the fact that the new lazystate
macros basically contain the same information as the lazystate enum. You could
fix this by making the enum use the macros.

    
    

    
  
maybe one solution is to keep the lazystate macros in nsXULElement.cpp (or .h),
but reserve space for the bits in nsGenericElement.h?

    
    

    
  


  
(In reply to comment #66)
> This way of doing the lazystat bits doesn't work.
oops.

        Attachment #170188 -
        Attachment is obsolete: true

        Attachment #170259 -
        Flags: superreview?(jst)

    
    

    
  
Comment on attachment 170259 [details] [diff] [review]
v 1.3 + comments from sicking, bz, bryner and peterv

it might be possible to use macros for part of the QI implementation. It's up
to you though.

        Attachment #170259 -
        Flags: review+

    
    

    
  
smaug, why should this block the release of 1.8a6? 

    
    

    
  
(In reply to comment #70)
> smaug, why should this block the release of 1.8a6? 
because this might need some testing, so alpha might be
better than beta ;) But actually I don't have strong opinion about 
that anymore.


    
    

    
  
Thanks, smaug. If the patch is fully reviewed and ready to land before we'll
ship, we will consider it for approval. Not going to block the release for it.
Flags: blocking1.8a6? → blocking1.8a6-

    
    

    
  
Comment on attachment 170259 [details] [diff] [review]
v 1.3 + comments from sicking, bz, bryner and peterv

 nsXULElement::~nsXULElement()
 {
+    //XXX SetDocument(nsnull) is not called always before dtor.
+    //XXX Related to templates or overlays?
+    SetDocument(nsnull, PR_TRUE, PR_FALSE);

Could this not be done conditionally if mDocument is non-null?

- In NS_NewXULElement():

+    // anchor the element so an early return will clean up properly.
+    nsCOMPtr<nsIContent> anchor = element;

That should be kungFuDeathGrip, not anchor ;)

All this looks really good to me, sr=jst, and thanks for the great
contribution!

        Attachment #170259 -
        Flags: superreview?(jst) → superreview+
Blocks: 265087

    
    

    
  
This was checked in last Friday.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
so were there code or performance improvements as a result? Would be nice to
have stats in the bug...

    
    

    
  
Tp was unaffected.  Ts and Txul both got a tiny bit slower (0.5-1% looks like).
 Codesize decreased by 5k.  The code was simplified quite a good bit, several
correctness issues were fixed, and the way was paved for other correctness
issues to be fixed.  There've been several more cleanup patches that have landed
since, removing code that is now duplicated, and that work is ongoing.
Blocks: 165110

    
  
Depends on: 401559

    
  
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general

    
    

    
  
Would Bug 1508119Remove XUL: Calendar/Lightning still uses XUL overlays which we've removed from TB help this bug at all?

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: