221335 - Speed up module output

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

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

There are three things I want to do to speed up module output:

1. Atomize txAXMLEventHandler::startElement and txAXMLEventHandler::attribute to
   avoid doing string->atom lookup (this now happens inside
   nsIDOMDocument::CreateElementNS and nsIDOMElement::SetAttribute)

2. Remove arguments to txAXMLEventHandler::endElement. This currently causes us
   to keep a stringstack and an int32 stack in txExecutionState. In module we
   don't need these arguments at all so it's a waste of cycles. In standalone
   we would need to keep a stringstack, but this can be kept in the right
   encoding, saving us the work to encode the end-tag.

3. Convert to using nsIContent as primary interface rather then nsIDOMNode. This
   will avoid a lot of overhead that nsIDOMNode::AppendChild has to do. Look at
   nsGenericElement::doInsertBefore for the extra work that goes on there.


Unfortunatly 1 comes with difficulties. First of all we in the common (html)
case need to lowercase the name that we get and then atomize the lowercase name.
So simply chaing to an atomized localname won't help. One way to solve this is
to add an optional |nsIAtom* aLowercaseLocalName| argument that is optional. For
LRE elements we can get this atom at compiletime. This actually goes well with
standalone as well. It too needs the lowercase nameatom for htmloutput.

However this doesn't solve things for xsl:element. Here we start out with the
stringname of the element so we don't want to get the atom just to have the
outputhandler convert that to a string again and lowercase that string. Instead
we should add an argument to supply the name in string-form. That way the
outputhandler can lowercase that string if needed, or just get the nameatom for
xml-output.

So we'd get a signature like this:
startElement(nsIAtom* aPrefix, nsIAtom* aLocalName, const nsAString& aName,
             PRInt32 aNsID, nsIAtom* aLowerCaseLocalName = nsnull);

where aPrefix+aLocalName is mutually exclusive with aName.

Comment 1

[Axel Hecht]

I'd say lets do 2 and 3 right away.
I'd prefer two entry points for 1, if we need two code paths.
We shouldn't get the lower-case atom for
- specified output method
- lower cased atoms (we might even check for plain lower case ASCII, as that
handles HTML gracefully)

Is aName a local name or a qname? (standalone would like qname, I guess)

Comment 2

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

Attachment: WIP 1

This is a work in process. It compiles and runs fine and does 2 and 3 but also
does part of 1. It was during development of this that I realized the
lowercase/xsl:element problem. Attaching for safekeeping

Comment 3

[Axel Hecht]

WAAAAAAAAAAAAAAAHHHHHHHHHHHHHHHHHH.
My eyes hurt. Badly. I looked at HTML content creation.
I need money. Badly. I gonna call my laywers and sue somebody. Time Warner is
soo in trouble, there is no disclaimer anywhere near that code.

nsHTMLDocument::CreateElement creates a nodeinfo and calls into 
NS_CreateHTMLElement. That is over in nsHTMLContentSink. NS_CreateHTMLElement
takes the atom and calls into the parser service to get the Id. The parser
service, fasten seatbelts here, latest, converts the atom to nsAutoString.
(We're in nsParserService::HTMLAtomTagToId or HTMLCaseSensitiveAtomTagToId, if
you still want to know:)
That calls into a hashtable to find the nsHTMLTag, or a PRUint32, or a PRInt32,
depending on where you look. If your eyes still work.
For plain html, that id is used to get back a string buffer. If we had to 
lowercase something, we get the atom again and put it back into the nodeinfo.
Now we're in MakeContentObject, which tries to SetForm on a few form controls
or just calls into NS_NewHTML##_classname##Element or
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLUnknownElement.cpp#78
Oh, yes, for the sake of it, nsHTMLDocument::CreateElement finally sets the
contentID. (That other, obsolete one.)

The html element factory calls into NS_CreateHTMLElement as well, btw.

Comment 4

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

Comment on attachment 132743 [details] [diff] [review]
WIP 1

note to self:

txMozillaXMLOutput::createElement should call SetContentID. And the
transformiix:result needs an id too.

Comment 5

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

Attachment: patch v1

this backs out the 1. part and just does 2. and 3

Comment 6

[Axel Hecht]

standalone:
kill txHTMLOutput::txCurrentElements, and drop the stack-one-string foo in 
txXMLOutput in favour of two string lists. One live, one recycle. Like
struct txCStringList {
  nsCAutoString mCString;
  txCStringList* mNext;
}
and use the mCString in html output to trigger the empty elems and attrs.
I'll attach some gperf based code to do this. That will do the lower casing
internally, so we never copy the string. Plus, it will work only with static
data, gonna be really sweet. Much better than the nsStaticNameTable.

Am I seeing this right, empty attrs were completely foobared?

mHaveDocumentElement is long dead, because we have txUnknownHandler? Or why
did it and it's related comments go?

module:
nsIContents should be named Content, not Node. I bet most of them don't need
to be addrefed.

+    PRPackedBool mCurrentIsDoc;
+    PRPackedBool mParentIsDoc;

why the ownership change in txMozillaTextOutput?
that should be an enum, as they're not independant.

The
ToLowerCase(aName, lowerName);
in txMozillaXMLOutput::startElement 
should only happen for legacy HTML output to XML documents. HTML (nsID=0) deals
with mixed cases itself. Badly, but that's a different bug.
I'll prolly port the fix in nsHTMLTokens to this place, too, so that we can
get the html atom cheap, and case insensitive if we need to.

+            nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(mDocument);
+            rv = domDoc->CreateElementNS(NS_LITERAL_STRING(kTXNameSpaceURI),
+                                         NS_LITERAL_STRING(kTXWrapper),
+                                         getter_AddRefs(wrapper));

Couldn't you just get the namespaceID for kTXNameSpaceURI and call 
txMozillaXMLOutput::createElement with aFactoryNsID=kNameSpaceID_None?
That should speed this up, too. (Not that we trigger it often, but anyway.)

I bet you don't need to addref in the following reparenting, too.

-        if (mDontAddCurrent && !mNonAddedParent) {
+        if (mDontAddCurrent && !mNonAddedNode) {
really?

+                    NS_ASSERTION(!mParentIsDoc, "failed to insert into document");
that one is in the else clause of mParentIsDoc.

-        rv = mDocument->CreateTextNode(mText, getter_AddRefs(text));
+        nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(mDocument);
+        rv = domDoc->CreateTextNode(mText, getter_AddRefs(text));

NS_NewTextNode(nsITextContent** aResult);
is part of nsITextContent, use that. The ownerDocument isn't set anyway.
(The XML content sink does this by hand, btw.) (*shiver*)
Same for comments.

PRBool aXHTML in startHTMLElement keeps confusing me. Someday we should rename
that to aDoTableFixup. Of course, the sensitive question is, does it depend
on the namespace of the current table or does it depend on the document we're
working on. Mixing HTML and XHTML content in one document confuses me, I can't
come up with a way I like it. Just adding this here for completeness, as I get
confused about it every time I look at it.

In the title fixup, I'd prefer to QI to nsIDOMCharacterData and use GetData.
One virtual function call less. No biggie.

As you change
doc->SetBaseURL(baseURI); // The document checks if it is legal...
anyway, move the content on a separate line to fix the linelength.

general:
why did you remove txUnknownHandler::attribute?

Comment 7

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

I don't understand the gpref comment, sounds more complicated then the currect code.

> Am I seeing this right, empty attrs were completely foobared?
afaict, yes

> mHaveDocumentElement is long dead, because we have txUnknownHandler? Or why
> did it and it's related comments go?
yes, txUnknownHandler takes care of all that nowadays.

> nsIContents should be named Content, not Node. I bet most of them don't need
> to be addrefed.
Hrm.. i'm not really happy about renaming mCurrentNode and mParentNode as i tend
to think of the objects as node even when held on through an nsIContent (is a
document-fragment content or a node?), but i guess i don't care that much.

Which don't need addrefing?

> +    PRPackedBool mCurrentIsDoc;
> +    PRPackedBool mParentIsDoc;
> that should be an enum, as they're not independant.

I'd rather stick to this since this is more clear.

> why the ownership change in txMozillaTextOutput?
The same change was made to the xml-output in bug 188729 but i forgot to do it
for textoutput then. Fixed this since i was already there.

> Couldn't you just get the namespaceID for kTXNameSpaceURI and call 
> txMozillaXMLOutput::createElement with aFactoryNsID=kNameSpaceID_None?
> That should speed this up, too. (Not that we trigger it often, but anyway.)
Yes, but it'd end up being more code then this, speed doesn't really matter
since we only do this at the most once per transformation.

> I bet you don't need to addref in the following reparenting, too.
Why wouldn't I?

> -        if (mDontAddCurrent && !mNonAddedParent) {
> +        if (mDontAddCurrent && !mNonAddedNode) {
> really?

yes, it didn't used to matter if we used mNonAddedParent or mNonAddedNode,
either both were set or both were null. However now the parent can be null if
it's the document-node.

> NS_NewTextNode(nsITextContent** aResult);
Err.. how can i call that without linking to gklayout.dll?

> why did you remove txUnknownHandler::attribute?
Because the code works just as fine without it

Comment 8

[Peter Van der Beken [:peterv]]

>> +    PRPackedBool mCurrentIsDoc;
>> +    PRPackedBool mParentIsDoc;
>> that should be an enum, as they're not independant.
>
>  I'd rather stick to this since this is more clear.

I think I agree with Pike on this.

Comment 9

[Peter Van der Beken [:peterv]]

> Am I seeing this right, empty attrs were completely foobared?

/me hides

Comment 10

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

> The
> ToLowerCase(aName, lowerName);
> in txMozillaXMLOutput::startElement 
> should only happen for legacy HTML output to XML documents. HTML (nsID=0) deals
> with mixed cases itself. Badly, but that's a different bug.

No, the bug is that the html-factory lowercases the 

>>> +    PRPackedBool mCurrentIsDoc;
>>> +    PRPackedBool mParentIsDoc;
>>> that should be an enum, as they're not independant.
>>  I'd rather stick to this since this is more clear.
>I think I agree with Pike on this.

mCurrentIsDoc indicates that mCurrentNode (now renamed mCurrentContent) should
point to the document-node but can't since documents arn't nsIContents.
mParentIsDoc is the same thing for mParentNode (now renamed mParentContent).

So the logic for the two are compleatly separated. Granted, both can't be true
at the same time, but if i put them together the logic for dealing with
mCurrentNode and mParentNode would be harder to read as i would basically have
to parse out the above two booleans from the enum.

Comment 11

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

Renaming them to mCurrentContentIsDoc and mParentContentIsDoc might be a good
idea though to indicate that they are locically connected to mCurrentContent
resp. mParentContent

Comment 12

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

> No, the bug is that the html-factory lowercases the...

upps.

The bug is that the html-factory lowercases the atom. That seems like a waste of
work and logically wrong since you'd get back a node with a different
nsINodeInfo then the one you requested.

Comment 13

[Peter Van der Beken [:peterv]]

BTW, you should check if we can now remove nsSupportsWeakReference from
nsContentSink.

Comment 14

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

Thinking some more i'll keep the ToLowerCase in there, if we get a non-lowercase
atom we'll end up forcing the creation of a new atom which will get put into the
atomhash, and then immideatly removed from the hash and deleted.

Keeping the ToLowerCase keeps the code doing what it did before so it should at
least not be slower. It would be nice with a string-based interface somewhere to
caseinsensitively get an though, but that's for later.

Comment 15

[Axel Hecht]

> No, the bug is that the html-factory lowercases the the atom.

It's not necessarily a bug, but it'd be unfortunate to rely on it. Looking 
closer, we miss the hash each time we create a miscased html element in the
nodeinfo manager. As that nodeinfo get's it's name atom changed, which calls into
the nodeinfo manager again.

Leave the code as is, but please add a comment that we should use the parser
service once that doesn't suck. It should be able to give a case-insensitive
atom, or even a case sensitive one. At a lower cost than going thru the global
atom table. But that's for later.

Comment 16

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

the nsScriptLoaderObserverProxy holds a weak reference to the nsContentSink so
we'll have to live with it still

Comment 17

[Axel Hecht]

gperf info:
http://www.gnu.org/software/gperf/gperf.html

It creates almost-perfect not-very-minimal hash functions. Those can be tweaked
to be pretty nice and case-insensitive, I bet even for both char and PRUnichar
strings. I'm kinda working on those in the background, exchanging mails with
alecf and dougt.

Comment 18

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

Attachment: patch v2

Addresses Pikes comments except for the ones i've commented on.

Comment 19

[Axel Hecht]

comment in shorter, I lost the long one.
The title comment was about using GetData, not just a different QI.

The refcounting stuff sounds like only orphaned nodes should be refcounted.
That is, mCurrentContent and mParentContent not, mNonAddedFoo should. Didn't 
apply the patch yet (this one ain't ready, IMHO), so can't check for sure.
(Note, TransformToFragment owns the fragment, the other calls generate a document
which should be owned either in the observer or in the output handler, not sure,
maybe just both. mCurrentContent anyway doesn't need to own.)

mParentContentIsDoc is wrong, the parent content is never a document. The
flag is obviously mParentIsDoc. Of course mNonAddedNode is a Content.

PRInt32 nameID;
rv = gTxNameSpaceManager->RegisterNameSpace(NS_LITERAL_STRING(kTXNameSpaceURI),
&nameID);
NS_ENSURE_SUCCESS(rv, rv);
nsCOMPtr<nsIAtom> lname = do_GetAtom(NS_LITERAL_CSTRING(kTXWrapper));

nsCOMPtr<nsIContent> wrapperCont;
rv = createElement(lname, nsnull, nameID, kNameSpaceID_None,
                   getter_AddRefs(wrapperCont));
NS_ENSURE_SUCCESS(rv, rv);

is longer than

nsCOMPtr<nsIDOMElement> wrapper;

nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(mDocument);
rv = domDoc->CreateElementNS(NS_LITERAL_STRING(kTXNameSpaceURI),
                             NS_LITERAL_STRING(kTXWrapper),
                             getter_AddRefs(wrapper));
NS_ASSERTION(NS_SUCCEEDED(rv), "Can't create wrapper element");

nsCOMPtr<nsIContent> wrapperCont = do_QueryInterface(wrapper);
?

Standalone issues still persist, esp the empty elem/attr foo and the end 
element tracking/buffering.

I'll attach a testcase to be checked with the txUnknownHandler stuff.

Comment 20

[Axel Hecht]

Attachment: bork a xsl:element to get xsl:attribute into the unknown handler, get html output nevertheless

odd, this hangs standalone. Will eventually investigate.
Trunk module works fine with this though, so make sure that it doesn't regress.

Comment 21

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

I want mParentContentIsDoc to have a name that connects it to mParentContent,
which is why i think mParentNode is a better name for it.

On createElementNS, the former is longer afaict, just as slow, and less pretty.
And it's only executed once per transformation.

And these comments are beyond nits. It's getting pretty annoying at this point.


What if a script move mCurrentContent out of the tree? Same for all other nodes. 

I'm not changeing to using that stuct. If you need that change when you do gperf
then you do it when you implement gperf. I'll look into if i can get rid of the
txCurrentElements stack without perfloss though

Comment 22

[Peter Van der Beken [:peterv]]

Quick opinions:
-I don't like mElementNames and mElementNameIndexes, what's wrong with a
nsCStringArray?
-Don't kill txUnknownHandler::attribute imho.

Let's hold off with changes and comments for now, I'll try to get through this
and give my comments and then we can weed out the real problems. Reviews are
important but both of you need to be more lenient towards a patch's writer.

Comment 23

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

The only reason i didn't go with nsCStringArray is that this is faster, the
array needs to allocate all the strings as separate objects (and separate
stringbuffers). I could create a txCStringStack wrapperclass though?

I don't understand the purpose of txUnknownHandler::attribute. The only thing
that does is ignore attributes before the first element, which
txBufferingHandler does too.


I looked into killing the currentElement-stack. It is possible, however i was
thinking that if we are going to support more encodings, such as UTF-16, as well
as possibly using these outputhandlers for module transformToString we aren't
going to be able to rely on the encoded string any more. Though we could always
revert the change at that time.
Oppinions?

Comment 24

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

Note to self: We could remove the |mCurrentContentIsDoc| and
|mParentContentIsDoc| flags in favour of other things to make the code more clear.

Rather then having mCurrentContentIsDoc we could have |mError|. The only time we
don't have a current-node is in error-coditions. Eventually we could get rid of
this member entierly once output-handlers return nsresult rather then void.

Rather then having |mParentContentIsDoc| we could have |mCurrentElementIsOpen|
that would indicate the the current-node is an element that has been created but
not yet "closed" (i.e. not inserted in the tree)

Comment 25

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

Comment on attachment 133884 [details] [diff] [review]
patch v2

ugh, this has major bitrot. I'll see if i can sync to tip.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Current profile of prettyprinting a largish XML file

Comment 27

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

Attachment: Patch to fix

This patch adds a couple of new methods to txAXMLEventHandler so that element and attribute names can be specified as atoms rather than strings. It also removes the arguments to endElement so that we don't have to push and pop the element names all the time.

I also made the output handlers use nsINode type methods rather than the generic DOM calls since that appears to speed things up a good bit.

I moved the check for valid node names into the txAXMLEventHandler so that we don't have to verify the name both in the caller and the callee.

The patch also changes some nsAString arguments to nsString, mostly because I needed to be able to call txXMLUtils::isValidQName.

I removed txUnknownHandler::attribute since the inherited method works fine and I didn't want to add the atomized ::attribute signature.

The one place I didn't convert to using nsINode-style methods is the code that creates the error-document. While it'd probably be good to do, it doesn't feel very critical so I'd rather do it at a later point if we feel like it.

This patch does not fix standalone, which I'd rather not deal with since it's pretty much unused anyway.


I ran this through buster without any new assertions or failures. (though currently trunk has a regression from bug 352583)

Comment 28

[Peter Van der Beken [:peterv]]

(In reply to comment #27)
> This patch adds a couple of new methods to txAXMLEventHandler so that element
> and attribute names can be specified as atoms rather than strings. It also
> removes the arguments to endElement so that we don't have to push and pop the
> element names all the time.

I think this is the wrong approach. We should have a method taking a txXPathNode  and an argument aDeep. We can then either just import the nsINode (if aDeep is PR_TRUE or not the node is not an element) or call a function that takes localname/prefix atoms. Then you can make the element handler taking localname/prefix strings call that function too.

> I moved the check for valid node names into the txAXMLEventHandler so that we
> don't have to verify the name both in the caller and the callee.

We shouldn't be doing the check if all we're doing is importing exisiting nodes.

> This patch does not fix standalone, which I'd rather not deal with since it's
> pretty much unused anyway.

I must say I don't like that very much.

Comment 29

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

(In reply to comment #28)
> (In reply to comment #27)
> > This patch adds a couple of new methods to txAXMLEventHandler so that element
> > and attribute names can be specified as atoms rather than strings. It also
> > removes the arguments to endElement so that we don't have to push and pop the
> > element names all the time.
> 
> I think this is the wrong approach. We should have a method taking a
> txXPathNode  and an argument aDeep. We can then either just import the nsINode
> (if aDeep is PR_TRUE or not the node is not an element) or call a function that
> takes localname/prefix atoms. Then you can make the element handler taking
> localname/prefix strings call that function too.

Most of the time we don't have a node, in txLREElement and txLREAttribute. For copy I guess we could do something like you describe, but that'd probably be a third startElement signature. This sounds like a good idea, but one that should be a separate patch.

> > I moved the check for valid node names into the txAXMLEventHandler so that we
> > don't have to verify the name both in the caller and the callee.
> 
> We shouldn't be doing the check if all we're doing is importing exisiting
> nodes.

With the patch we only check the name when using <xsl:element> in which case we have to since all we're given is a string.

> > This patch does not fix standalone, which I'd rather not deal with since it's
> > pretty much unused anyway.
> 
> I must say I don't like that very much.

I'm not exited about it either, but I don't feel I have the cycles to do it.

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 238729 [details] [diff] [review]
Patch to fix

>Index: content/xml/document/src/nsXMLContentSink.cpp
>===================================================================

>-nsXMLContentSink::OnDocumentCreated(nsIDOMDocument* aResultDocument)
>+nsXMLContentSink::OnDocumentCreated(nsIDocument* aResultDocument)

>-    return contentViewer->SetDOMDocument(aResultDocument);
>+    nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aResultDocument);
>+    return contentViewer->SetDOMDocument(doc);

Make SetDOMDocument take nsIDocument.

>Index: content/xslt/src/xslt/txInstructions.cpp
>===================================================================

> txAttribute::execute(txExecutionState& aEs)
> {
>     nsAutoString name;
>     nsresult rv = mName->evaluateToString(aEs.getEvalContext(), name);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    const PRUnichar* colon;
>-    if (!XMLUtils::isValidQName(name, &colon) ||
>-        TX_StringEqualsAtom(name, txXMLAtoms::xmlns)) {
>-        // truncate name to indicate failure
>-        name.Truncate();
>-    }

Don't remove this call to isValidQName. Instead make the handlers take nsIAtom* aPrefix and nsString aLocalName.

>+    // add attribute if everything was ok
>+    return nsId != kNameSpaceID_None ?
>+           aEs.mResultHandler->attribute(name, nsId, handler->mValue) :
>+           NS_OK;

You're dropping attributes with no namespace?

>@@ -294,44 +278,51 @@ txCopyBase::copyNode(const txXPathNode& 

>         case txXPathNodeType::ELEMENT_NODE:
>         {
>-            nsAutoString name;
>-            txXPathNodeUtils::getNodeName(aNode, name);
>-            PRInt32 nsID = txXPathNodeUtils::getNamespaceID(aNode);
>-            nsresult rv = aEs.mResultHandler->startElement(name, nsID);
>+            nsCOMPtr<nsIAtom> localName =
>+                txXPathNodeUtils::getLocalName(aNode);
>+            nsresult rv = aEs.mResultHandler->
>+                startElement(txXPathNodeUtils::getPrefix(aNode),
>+                             localName, nsnull,
>+                             txXPathNodeUtils::getNamespaceID(aNode));
>             NS_ENSURE_SUCCESS(rv, rv);

Don't you need to push a PR_TRUE on the bool stack here?

> 
>             // Copy attributes
>             txXPathTreeWalker walker(aNode);
>             if (walker.moveToFirstAttribute()) {
>                 do {
>-                    nsAutoString nodeName, nodeValue;
>-                    walker.getNodeName(nodeName);
>+                    nsAutoString nodeValue;
>                     walker.appendNodeValue(nodeValue);
>+
>+                    const txXPathNode& attr = walker.getCurrentPosition();
>+                    localName = txXPathNodeUtils::getLocalName(attr);
>                     rv = aEs.mResultHandler->
>-                      attribute(nodeName, walker.getNamespaceID(), nodeValue);
>+                        attribute(txXPathNodeUtils::getPrefix(attr),
>+                                  localName, nsnull,
>+                                  txXPathNodeUtils::getNamespaceID(attr),
>+                                  nodeValue);
>                     NS_ENSURE_SUCCESS(rv, rv);
>                 } while (walker.moveToNextAttribute());
>                 walker.moveToParent();
>             }
> 
>             // Copy children
>             PRBool hasChild = walker.moveToFirstChild();
>             while (hasChild) {
>                 copyNode(walker.getCurrentPosition(), aEs);
>                 hasChild = walker.moveToNextSibling();
>             }
> 
>-            return aEs.mResultHandler->endElement(name, nsID);
>+            return aEs.mResultHandler->endElement();

I'd prefer if you'd drop these changes, it'll be handled when we add a method to the output handler that calls importNode.

>@@ -450,24 +437,18 @@ txCopyOf::execute(txExecutionState& aEs)

> txEndElement::execute(txExecutionState& aEs)

>-    // For xsl:elements with a bad name we push an empty name
>-    if (!nodeName.IsEmpty()) {
>-        return aEs.mResultHandler->endElement(nodeName, namespaceID);
>+    if (aEs.popBool()) {

Leave some comment here about xsl:element and a bad name.

>@@ -530,47 +511,42 @@ txLoopNodeSet::execute(txExecutionState&

> txLREAttribute::txLREAttribute(PRInt32 aNamespaceID, nsIAtom* aLocalName,

>+    nsAutoString lname;
>+    aLocalName->ToString(lname);
>+    TX_ToLowerCase(lname);
>+    mLowercaseLocalName = do_GetAtom(lname);

Add a TX_ToLowerCase(nsIAtom*, nsIAtom**) and avoid the conversion from/to UTF-8 by using GetUTF8String.
Wouldn't it be better to only do this for kNameSpaceID_None?
  
> nsresult
> txStartElement::execute(txExecutionState& aEs)
> {
>     nsAutoString name;
>     nsresult rv = mName->evaluateToString(aEs.getEvalContext(), name);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    const PRUnichar* colon;
>-    if (!XMLUtils::isValidQName(name, &colon)) {
>-        // truncate name to indicate failure
>-        name.Truncate();
>-    }

Don't remove this call to isValidQName. Instead make the handlers take nsIAtom* aPrefix and nsString aLocalName.

> txStartLREElement::txStartLREElement(PRInt32 aNamespaceID,
>                                      nsIAtom* aLocalName,
>                                      nsIAtom* aPrefix)
>     : mNamespaceID(aNamespaceID),
>       mLocalName(aLocalName),
>       mPrefix(aPrefix)
> {
>+    nsAutoString lname;
>+    aLocalName->ToString(lname);
>+    TX_ToLowerCase(lname);
>+    mLowercaseLocalName = do_GetAtom(lname);

TX_ToLowerCase and only for kNameSpaceID_None.

>Index: content/xslt/src/xslt/txMozillaTextOutput.cpp
>===================================================================

> txMozillaTextOutput::endDocument(nsresult aResult)

>+    text->SetText(mText, PR_FALSE);

Need to check result of this call.

>@@ -151,150 +154,146 @@ txMozillaTextOutput::createResultDocumen

>+    

Don't add spaces on empty lines.

>Index: content/xslt/src/xslt/txMozillaXMLOutput.cpp
>===================================================================

>-txMozillaXMLOutput::txMozillaXMLOutput(const nsAString& aRootName,
>+txMozillaXMLOutput::txMozillaXMLOutput(const nsString& aRootName,
>                                        PRInt32 aRootNsID,
>                                        txOutputFormat* aFormat,
>                                        nsIDOMDocument* aSourceDocument,
>                                        nsIDOMDocument* aResultDocument,
>                                        nsITransformObserver* aObserver)
>     : mTreeDepth(0),
>       mBadChildLevel(0),
>       mTableState(NORMAL),
>       mDontAddCurrent(PR_FALSE),
>       mHaveTitleElement(PR_FALSE),
>       mHaveBaseElement(PR_FALSE),
>       mCreatingNewDocument(PR_TRUE),
>+      mOpenedElementIsHTML(PR_FALSE),
>       mRootContentCreated(PR_FALSE)
> {
>     if (aObserver) {
>         mNotifier = new txTransformNotifier();
>         if (mNotifier) {
>             mNotifier->Init(aObserver);
>         }
>     }
>@@ -116,113 +109,173 @@ txMozillaXMLOutput::txMozillaXMLOutput(c
> txMozillaXMLOutput::txMozillaXMLOutput(txOutputFormat* aFormat,
>                                        nsIDOMDocumentFragment* aFragment)
>     : mTreeDepth(0),
>       mBadChildLevel(0),
>       mTableState(NORMAL),
>       mDontAddCurrent(PR_FALSE),
>       mHaveTitleElement(PR_FALSE),
>       mHaveBaseElement(PR_FALSE),
>-      mCreatingNewDocument(PR_FALSE)
>+      mCreatingNewDocument(PR_FALSE),
>+      mOpenedElementIsHTML(PR_FALSE),
>+      mRootContentCreated(PR_FALSE)
> {
>     mOutputFormat.merge(*aFormat);
>     mOutputFormat.setFromDefaults();
> 
>-    aFragment->GetOwnerDocument(getter_AddRefs(mDocument));
>-
>-    nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocument);
>-    mDocumentIsHTML = doc && !doc->IsCaseSensitive();
>-
>-    mCurrentNode = aFragment;
>+    mCurrentNode = do_QueryInterface(aFragment);
>+    mDocument = mCurrentNode->GetOwnerDoc();
>+    if (!mDocument) {
>+      mCurrentNode = nsnull;
>+    }
> }
> 
> txMozillaXMLOutput::~txMozillaXMLOutput()
> {
> }
> 
> nsresult
>-txMozillaXMLOutput::attribute(const nsAString& aName,
>+txMozillaXMLOutput::attribute(nsIAtom* aPrefix,
>+                              nsIAtom* aLocalName,
>+                              nsIAtom* aLowercaseLocalName,
>                               const PRInt32 aNsID,
>-                              const nsAString& aValue)
>+                              const nsString& aValue)
> {
>-    nsresult rv;
>-    if (!mParentNode)
>-        // XXX Signal this? (can't add attributes after element closed)
>-        return NS_OK;
>+    nsCOMPtr<nsIAtom> owner;
>+    if (mOpenedElementIsHTML && aNsID == kNameSpaceID_None) {
>+        if (aLowercaseLocalName) {
>+            aLocalName = aLowercaseLocalName;
>+        }
>+        else {
>+            nsAutoString lname;
>+            aLocalName->ToString(lname);
>+            ToLowerCase(lname);
>+            owner = do_GetAtom(lname);
>+            NS_ENSURE_TRUE(owner, NS_ERROR_OUT_OF_MEMORY);
> 
>-    if (mBadChildLevel) {
>+            aLocalName = owner;
>+        }
>+    }
>+
>+    return attributeInternal(aPrefix, aLocalName, aNsID, aValue);
>+}
>+
>+nsresult
>+txMozillaXMLOutput::attribute(const nsString& aName,

>+        if (colon) {
>+            ToLowerCase(Substring(aName.get(), colon), lnameStr);

This puts the lowercase prefix in lnameStr.

>+    NS_ENSURE_TRUE(lname, NS_ERROR_OUT_OF_MEMORY);

  if (!lname) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

>+    // Check that it's a valid name
>+    if (!nsContentUtils::IsValidNodeName(lname, prefix, aNsID)) {
>+        // Try without prefix

Why? I don't think we should let that slip.

>+txMozillaXMLOutput::attributeInternal(nsIAtom* aPrefix,

>+    return mOpenedElement->SetAttr(aNsID, aLocalName, aPrefix, aValue, PR_FALSE);

Long line.

> void txMozillaXMLOutput::getOutputDocument(nsIDOMDocument** aDocument)
> {
>-    *aDocument = mDocument;
>-    NS_IF_ADDREF(*aDocument);
>+    CallQueryInterface(mDocument, aDocument);

getOutputDocument should return nsresult now.

>@@ -417,313 +457,360 @@ txMozillaXMLOutput::startDocument()

>+txMozillaXMLOutput::startElement(nsIAtom* aPrefix, nsIAtom* aLocalName,
>+                                 nsIAtom* aLowercaseLocalName, const PRInt32 aNsID)

Long line.

>+txMozillaXMLOutput::startElement(const nsString& aName, const PRInt32 aNsID)

>+        if (colon) {
>+            ToLowerCase(Substring(aName.get(), colon), lnameStr);

This puts the lowercase prefix in lnameStr.

>+            lname = do_GetAtom(Substring(colon + 1, aName.get() + aName.Length()));

Long line.

>+    if (!nsContentUtils::IsValidNodeName(lname, prefix, aNsID)) {
>+        // Try without prefix

Why? I don't think we should let that slip.

>+txMozillaXMLOutput::startElementInternal(nsIAtom* aPrefix,

>-    if ((mOutputFormat.mMethod == eHTMLOutput) && (aNsID == kNameSpaceID_None)) {
>-        if (mDocumentIsHTML) {
>-            rv = mDocument->CreateElement(aName,
>-                                          getter_AddRefs(element));
>-        }
>-        else {
>-            nsAutoString lcname;
>-            ToLowerCase(aName, lcname);
>-            rv = mDocument->CreateElementNS(NS_LITERAL_STRING(kXHTMLNameSpaceURI),
>-                                            lcname,
>-                                            getter_AddRefs(element));
>-        }
>-        NS_ENSURE_SUCCESS(rv, rv);
>+    // Create the element
>+    nsCOMPtr<nsINodeInfo> ni;
>+    rv = mNodeInfoManager->GetNodeInfo(aLocalName, aPrefix, aNsID,
>+                                       getter_AddRefs(ni));
>+    NS_ENSURE_SUCCESS(rv, rv);

So this will create HTML elements in the null namespace in an XML document?
(if mOutputFormat.mMethod == eHTMLOutput && aNsID == kNameSpaceID_None && !mDocumentIsHTML)

>+        mOpenedElementIsHTML = aNsID != kNameSpaceID_XHTML;
>+        rv = startHTMLElement(mOpenedElement, aNsID == kNameSpaceID_XHTML);

Seems startHTMLElement should check mOpenedElementIsHTML instead of having a second argument.

>+    else if (aNsID == kNameSpaceID_SVG &&
>+             aLocalName == txHTMLAtoms::script) {

Rewrap.

> txMozillaXMLOutput::createTxWrapper()

>+            rootLocation = PR_MAX(rootLocation, j+1);

Add some space around the + while you're here.

>@@ -891,58 +969,72 @@ txMozillaXMLOutput::createResultDocument

>+            nsCOMPtr<nsIAtom> doctypeName = do_GetAtom(qName);
>+            NS_ENSURE_TRUE(doctypeName, NS_ERROR_OUT_OF_MEMORY);

  if (!doctypeName) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

>+            rv = NS_NewDOMDocumentType(getter_AddRefs(documentType),
>+                                       mNodeInfoManager,
>+                                       nsnull,
>+                                       doctypeName, nsnull, nsnull,
>+                                       mOutputFormat.mPublicId,
>+                                       mOutputFormat.mSystemId,
>+                                       EmptyString());

Rewrap.

>+txMozillaXMLOutput::createHTMLElement(nsIAtom* aName,
>+                                      nsIContent** aResult)
> {
>-    if (mDocumentIsHTML) {
>-        return mDocument->CreateElement(aName, aResult);
>-    }
>+    NS_ASSERTION(mOutputFormat.mMethod == eHTMLOutput,
>+                 "need to adjust createHTMLElement");

There's callers to createHTMLElement that don't ensure this.

>Index: content/xslt/src/xslt/txMozillaXMLOutput.h
>===================================================================

>+    nsCOMPtr<nsINode> mCurrentNode;
>+    nsCOMPtr<nsIContent> mOpenedElement;

Document that if mOpenedElement is non-null mCurrentNode is not the current node.

>-    nsCOMPtr<nsIDOMNode> mNonAddedParent;

>+    nsCOMArray<nsINode> mCurrentNodeStack;

I don't see a need for replacing mNonAddedParent with mCurrentNodeStack.

>-    PRPackedBool mDocumentIsHTML;

Please explain why this wouldn't be needed anymore (see also the comment in txMozillaXMLOutput::startElementInternal).

>Index: content/xslt/src/xslt/txUnknownHandler.cpp
>===================================================================

>+txUnknownHandler::startElement(nsIAtom* aPrefix, nsIAtom* aName,

>-    txOutputFormat* format = mEs->mStylesheet->getOutputFormat();
>-    if (format->mMethod != eMethodNotSet) {

At least assert this if you remove it.

>+nsresult txUnknownHandler::createHandlerAndFlush(PRBool aHTMLRoot,

>+    txOutputFormat* setFormat = mEs->mStylesheet->getOutputFormat();
>+
>     txOutputFormat format;
>-    format.merge(*mEs->mStylesheet->getOutputFormat());

>+    format.merge(*setFormat);

Undo this change.

Comment 31

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

(In reply to comment #30)
> (From update of attachment 238729 [details] [diff] [review] [edit])
> >Index: content/xml/document/src/nsXMLContentSink.cpp
> >===================================================================
> 
> >-nsXMLContentSink::OnDocumentCreated(nsIDOMDocument* aResultDocument)
> >+nsXMLContentSink::OnDocumentCreated(nsIDocument* aResultDocument)
> 
> >-    return contentViewer->SetDOMDocument(aResultDocument);
> >+    nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aResultDocument);
> >+    return contentViewer->SetDOMDocument(doc);
> 
> Make SetDOMDocument take nsIDocument.

nsIContentViewer is scriptable so I can't do that.

> >Index: content/xslt/src/xslt/txInstructions.cpp
> >===================================================================
> 
> > txAttribute::execute(txExecutionState& aEs)
> > {
> >     nsAutoString name;
> >     nsresult rv = mName->evaluateToString(aEs.getEvalContext(), name);
> >     NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-    const PRUnichar* colon;
> >-    if (!XMLUtils::isValidQName(name, &colon) ||
> >-        TX_StringEqualsAtom(name, txXMLAtoms::xmlns)) {
> >-        // truncate name to indicate failure
> >-        name.Truncate();
> >-    }
> 
> Don't remove this call to isValidQName. Instead make the handlers take nsIAtom*
> aPrefix and nsString aLocalName.

Done. An alternative is to just give startElement a string and a txNamespaceMap. However I think that pushes a little too much logic into the output handler.

> >+    // add attribute if everything was ok
> >+    return nsId != kNameSpaceID_None ?
> >+           aEs.mResultHandler->attribute(name, nsId, handler->mValue) :
> >+           NS_OK;
> 
> You're dropping attributes with no namespace?

Opps, that should be kNameSpaceID_Unknown. Strange that none of the xalan tests cought that.

> >@@ -294,44 +278,51 @@ txCopyBase::copyNode(const txXPathNode& 
> 
> >         case txXPathNodeType::ELEMENT_NODE:
> >         {
> >-            nsAutoString name;
> >-            txXPathNodeUtils::getNodeName(aNode, name);
> >-            PRInt32 nsID = txXPathNodeUtils::getNamespaceID(aNode);
> >-            nsresult rv = aEs.mResultHandler->startElement(name, nsID);
> >+            nsCOMPtr<nsIAtom> localName =
> >+                txXPathNodeUtils::getLocalName(aNode);
> >+            nsresult rv = aEs.mResultHandler->
> >+                startElement(txXPathNodeUtils::getPrefix(aNode),
> >+                             localName, nsnull,
> >+                             txXPathNodeUtils::getNamespaceID(aNode));
> >             NS_ENSURE_SUCCESS(rv, rv);
> 
> Don't you need to push a PR_TRUE on the bool stack here?

The bool stack is only there so that txEndElement knows if it should call handler->endElement() or not. So it's not needed here since we know that we've always called startElement.

> I'd prefer if you'd drop these changes, it'll be handled when we add a method
> to the output handler that calls importNode.

I'm not fully sure that that is a useful optimization given that the most common case is creating html, in which case the element-to-copy is a null-namespace XML element which we can't clone.

In any case, I'm not sure when such a new startElement signature will be added and I think we should do our best until then.


> >@@ -530,47 +511,42 @@ txLoopNodeSet::execute(txExecutionState&
> 
> > txLREAttribute::txLREAttribute(PRInt32 aNamespaceID, nsIAtom* aLocalName,
> 
> >+    nsAutoString lname;
> >+    aLocalName->ToString(lname);
> >+    TX_ToLowerCase(lname);
> >+    mLowercaseLocalName = do_GetAtom(lname);
> 
> Add a TX_ToLowerCase(nsIAtom*, nsIAtom**) and avoid the conversion from/to
> UTF-8 by using GetUTF8String.

Unfortunately there is no i18n aware ToLowerCase that operates on UTF8, so we have to convert to UTF16 no matter what. Is it really worth adding a function for just 4 lines of code?

> Wouldn't it be better to only do this for kNameSpaceID_None?

Done.

> >+    // Check that it's a valid name
> >+    if (!nsContentUtils::IsValidNodeName(lname, prefix, aNsID)) {
> >+        // Try without prefix
> 
> Why? I don't think we should let that slip.

The spec explicitly allows us to do this. We used to do it only for "xmlns" prefixes, but with this we'll do it in a few more situations (like "xml" prefixes).

> > void txMozillaXMLOutput::getOutputDocument(nsIDOMDocument** aDocument)
> > {
> >-    *aDocument = mDocument;
> >-    NS_IF_ADDREF(*aDocument);
> >+    CallQueryInterface(mDocument, aDocument);
> 
> getOutputDocument should return nsresult now.

The QI should never ever fail. We do similar QIs to nsIContent elsewhere.

> >+txMozillaXMLOutput::startElementInternal(nsIAtom* aPrefix,
> 
> >-    if ((mOutputFormat.mMethod == eHTMLOutput) && (aNsID == kNameSpaceID_None)) {
> >-        if (mDocumentIsHTML) {
> >-            rv = mDocument->CreateElement(aName,
> >-                                          getter_AddRefs(element));
> >-        }
> >-        else {
> >-            nsAutoString lcname;
> >-            ToLowerCase(aName, lcname);
> >-            rv = mDocument->CreateElementNS(NS_LITERAL_STRING(kXHTMLNameSpaceURI),
> >-                                            lcname,
> >-                                            getter_AddRefs(element));
> >-        }
> >-        NS_ENSURE_SUCCESS(rv, rv);
> >+    // Create the element
> >+    nsCOMPtr<nsINodeInfo> ni;
> >+    rv = mNodeInfoManager->GetNodeInfo(aLocalName, aPrefix, aNsID,
> >+                                       getter_AddRefs(ni));
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> So this will create HTML elements in the null namespace in an XML document?
> (if mOutputFormat.mMethod == eHTMLOutput && aNsID == kNameSpaceID_None &&
> !mDocumentIsHTML)

I'm not really sure what the right thing to do is, but that was the intention, yes. I think that if a user asks to create HTML elements we should do that and not create XHTML elements since they do behave different. The user might be moving these nodes into a HTML document later for all we know.

The only two ways this can happen is if either the user uses the old deprecated nsIXSLTProcessorObsolete::transformDocument function, or when using transformToFragment and passing in an XML document as creator-document and explicitly specifying HTML as output-method.

Unrelated; maybe we should consider killing the old nsIXSLTProcessorObsolete::transformDocument function.

> >+        mOpenedElementIsHTML = aNsID != kNameSpaceID_XHTML;
> >+        rv = startHTMLElement(mOpenedElement, aNsID == kNameSpaceID_XHTML);
> 
> Seems startHTMLElement should check mOpenedElementIsHTML instead of having a
> second argument.

I don't feel strongly either way, but shouldn't we then also not pass in mOpenedElement? Alternativly we could simply pass mOpendElementIsHTML (most of the time we use !aXHTML anyway)

> >@@ -891,58 +969,72 @@ txMozillaXMLOutput::createResultDocument
> 
> >+            nsCOMPtr<nsIAtom> doctypeName = do_GetAtom(qName);
> >+            NS_ENSURE_TRUE(doctypeName, NS_ERROR_OUT_OF_MEMORY);
> 
>   if (!doctypeName) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }

I prefer using NS_ENSURE_TRUE here, IMHO that's more readable

> >+txMozillaXMLOutput::createHTMLElement(nsIAtom* aName,
> >+                                      nsIContent** aResult)
> > {
> >-    if (mDocumentIsHTML) {
> >-        return mDocument->CreateElement(aName, aResult);
> >-    }
> >+    NS_ASSERTION(mOutputFormat.mMethod == eHTMLOutput,
> >+                 "need to adjust createHTMLElement");
> 
> There's callers to createHTMLElement that don't ensure this.

One caller is only used when we've just created an HTML element, which we should only be doing in HTML mode. The other caller is in an explicit |if (mMethod == eHTMLOutput)|

> >-    nsCOMPtr<nsIDOMNode> mNonAddedParent;
> >+    nsCOMArray<nsINode> mCurrentNodeStack;
> 
> I don't see a need for replacing mNonAddedParent with mCurrentNodeStack.

Actually, this change is not related to mNonAddedParent. The stack is there for us to deal better with someone mutating the result DOM in the middle of the transformation. I.e. if someone moves the current elemenet somewhere else in the tree we'll still be able to insert thing in the 'right' place once we step back up the tree.

This is what the content sinks do.

> >-    PRPackedBool mDocumentIsHTML;
> 
> Please explain why this wouldn't be needed anymore (see also the comment in
> txMozillaXMLOutput::startElementInternal).

We used to use it since before we were included in gklayout the only way we had of creating HTML elements was to call createElement on a HTML document. When we didn't have a HTML document we would create XHTML elements. Now that we live in gklayout we can always create HTML elements using NS_NewHTMLElement, so no need to go through the document and so we don't need to care what type it is.

> >Index: content/xslt/src/xslt/txUnknownHandler.cpp
> >===================================================================
> 
> >+txUnknownHandler::startElement(nsIAtom* aPrefix, nsIAtom* aName,
> 
> >-    txOutputFormat* format = mEs->mStylesheet->getOutputFormat();
> >-    if (format->mMethod != eMethodNotSet) {
> 
> At least assert this if you remove it.

This logic is moved to createHandlerAndFlush. The logic is actually needed because we do use txUnknownHandler for xml output so that we have the root node name by the time we create the document.

Comment 32

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

Attachment: Comments addressed

This addressed all comments not commented on above. I did add the TX_ToLowerCastAtom function though with a decomtaminated signature.

Comment 33

[Peter Van der Beken [:peterv]]

Comment on attachment 239271 [details] [diff] [review]
Comments addressed

(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 238729 [details] [diff] [review] [edit] [edit])

> Unrelated; maybe we should consider killing the old
> nsIXSLTProcessorObsolete::transformDocument function.

Yeah, for 1.9 that seems fine.

> > >+        mOpenedElementIsHTML = aNsID != kNameSpaceID_XHTML;
> > >+        rv = startHTMLElement(mOpenedElement, aNsID == kNameSpaceID_XHTML);
> > 
> > Seems startHTMLElement should check mOpenedElementIsHTML instead of having a
> > second argument.
> 
> I don't feel strongly either way, but shouldn't we then also not pass in
> mOpenedElement? Alternativly we could simply pass mOpendElementIsHTML (most of
> the time we use !aXHTML anyway)

Yeah, maybe the latter then.

> > >@@ -891,58 +969,72 @@ txMozillaXMLOutput::createResultDocument
> > 
> > >+            nsCOMPtr<nsIAtom> doctypeName = do_GetAtom(qName);
> > >+            NS_ENSURE_TRUE(doctypeName, NS_ERROR_OUT_OF_MEMORY);
> > 
> >   if (!doctypeName) {
> >     return NS_ERROR_OUT_OF_MEMORY;
> >   }
> 
> I prefer using NS_ENSURE_TRUE here, IMHO that's more readable

I know, but it's not the style of the file so please change it (and others).

>Index: content/xslt/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/content/xslt/Makefile.in,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 Makefile.in
>--- content/xslt/Makefile.in	13 Dec 2005 22:07:35 -0000	1.6
>+++ content/xslt/Makefile.in	19 Sep 2006 23:52:56 -0000
>@@ -39,13 +39,11 @@ DEPTH = ../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> DIRS = public src
> 
>-ifdef ENABLE_TESTS
> DIRS += tests/buster
>-endif
> 
> include $(topsrcdir)/config/rules.mk

Huh?

>Index: content/xslt/src/xslt/txInstructions.cpp
>===================================================================

>+    else {
>+        if (colon) {

    else if (colon) {

>Index: content/xslt/src/xslt/txMozillaXMLOutput.cpp
>===================================================================

>@@ -325,79 +346,80 @@ txMozillaXMLOutput::endElement(const nsA

>+        // Check to make sure that script hasn't inserted the node somewhere
>+        // else in the tree

Not really sure how script could get at mNonAddedNode :-).

>+        if (!mCurrentNode->GetNodeParent()) {
>+            parent->AppendChildTo(mNonAddedNode, PR_TRUE);
>+        }

> nsresult
> txMozillaXMLOutput::createTxWrapper()

>+            // Fall through

Drop that line.

>Index: content/xslt/src/xslt/txUnknownHandler.cpp
>===================================================================

>+txUnknownHandler::startElement(nsIAtom* aPrefix, nsIAtom* aLocalName,
>+                               nsIAtom* aLowercaseLocalName, PRInt32 aNsID)

>+    nsAutoString name;
>+    aLocalName->ToString(name);

Add a comment here to explain that we use aLocalName and not aLowercaseLocalName because if eHTMLOutput we'll hardcode it to html.

>Index: content/xslt/src/xslt/txXMLEventHandler.h
>===================================================================

>      * @param aName the name of the attribute
>      * @param aNsID the namespace ID of the attribute
>      * @param aValue the value of the attribute
>      */
>-    virtual nsresult attribute(const nsAString& aName,
>+    virtual nsresult attribute(nsIAtom* aPrefix,
>+                               const nsSubstring& aLocalName,

Update the description of the arguments (there's others like this).

Looks good!

Comment 34

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

> >+        // Check to make sure that script hasn't inserted the node somewhere
> >+        // else in the tree
> Not really sure how script could get at mNonAddedNode :-).

I suspect that if you add mutation-listener attributes to a child of a <script> then those listeners could fire and could walk up the parent chain to get to the script.

In any event, since I'm not sure either way i'd rather stay on the safe side.

Comment 35

[Boris Zbarsky [:bzbarsky]]

This introduced crash bug 355246.