227003 - add ::IsContentOfType to txXPathNode, txNameTest, txNodeTypeTest

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Axel Hecht]

txStylesheet::isStripSpaceAllowed, txNameTest::matches and txNodeTypeTest::matches
shouldn't call into getNodeType(), but use a new function going down into
nsIContent::IsContentOfType(), that gets rid of a QI with addrefs and stuff.

Assign this bug to me, if you want me to do it.

Comment 1

[Axel Hecht]

Jonas wrote in bug 226124:
We should have an txXPathNodeUtils::isOfType that tests if a node is a certain
type. Module can do that a lot faster then getting the node-type. If we want it
really fast we can even have ::isElement, ::isAttribute etc functions. Another
way of doing it is to have an inlined ::isOfType and rely on the optimizer to
optimize things into the same code as ::isElement etc.

I'm thinking in terms of isDocument, isAttribute and isContentOfType.
We have the first two for module already, we just need to port them to standalone.
The last one is a shallow (inlined) wrapper to IsContentOfType, to deal with 
text, elems, comments and pis. Standalone has to map the flags to nodetypes,
but that ain't bad. I'd rather have standalone pay a few lines of code (in
the current content model) than the other way around.

Comment 2

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

Attachment: just a draft

This is just the result of some testing i did a bit back. It is by no means
ment for review, just as a potential base for anyone that wants to fix this
bug.

I first tried the inlined-function approach (txXPathUtils::isOfType, ment to be
called with a constant so that the optimizer can optimize away the unneccesary
tests). However MSVC considered the function to be too big and wouldn't inline
it at all, which actually might not matter. Didn't test what gcc did.

To force inlineing i tested a macro and it worked fine, MSVC inlined and
optimized to exactly what we want. However it forced me to remove the |private|
on txXPathNode which we don't want to do. Another solution might be to call
some function in txXPathNativeNode to get the private information.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

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

Comment on attachment 171084 [details] [diff] [review]
v1

>Index: extensions/transformiix/source/xpath/txXPathTreeWalker.h
...
>+txXPathNodeUtils::localNameEquals(const txXPathNode& aNode,
>+                                  nsIAtom* aLocalName)
...
>+    nsCOMPtr<nsIAtom> localName = txXPathNodeUtils::getLocalName(aNode);
>+
>+    return localName == aLocalName;

If you add a section for isAttribute too then this is only needed for PIs. All
other nodetypes will return false.

>+inline PRBool
>+txXPathNodeUtils::isComment(const txXPathNode& aNode)
>+{
>+#ifdef TX_EXE
>+    return aNode.mInner->getNodeType() == Node::COMMENT_NODE;
>+#else
>+    PRBool result = PR_FALSE;
>+    if (aNode.isContent()) {
>+        nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aNode.mContent);
>+        PRUint16 type;
>+        node->GetNodeType(&type);
>+        result = type == txXPathNodeType::COMMENT_NODE;
>+    }

You can do this faster by checking what mContent->Tag() returns.

Is there a reason the txXPathNodeUtils/txXPathTreeWalker::getNodeType
implementations aren't removed?

With that fixed, r=me

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> >+inline PRBool
> >+txXPathNodeUtils::isComment(const txXPathNode& aNode)

> You can do this faster by checking what mContent->Tag() returns.

I ended up just adding eCOMMENT to IsContentOfType.

> Is there a reason the txXPathNodeUtils/txXPathTreeWalker::getNodeType
> implementations aren't removed?

txCopyBase::copyNode and txStylesheet::findTemplate both have a switch on
nodetype, I'm not sure I want to change them. I did remove
txXPathTreeWalker::getNodeType.

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> If you add a section for isAttribute too then this is only needed for PIs. All
> other nodetypes will return false.

Do you mean something like this?

    nsCOMPtr<nsIAtom> localName;
    if (aNode.isContent()) {
        nsINodeInfo* ni = aNode.mContent->GetNodeInfo();
        if (ni) {
            return ni->Equals(aLocalName);
        }
        if (!aNode.mContent->IsContentOfType(nsIContent::ePROCESSING_INSTRUCTION)) {
            return PR_FALSE;
        }
        localName = txXPathNodeUtils::getLocalName(aNode);
    }
    else if (aNode.isAttribute()) {
        nsCOMPtr<nsIAtom> prefix;
        PRInt32 namespaceID;
        aNode.mContent->GetAttrNameAt(aNode.mIndex, &namespaceID,
                                      getter_AddRefs(localName),
                                      getter_AddRefs(prefix));
    }

    return localName == aLocalName;


It's a bit of code duplication though :-/.

Comment 7

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

True. While this function probably is excersied often enough that it might be
worth to optimize hard, the vastly most common case is probably that it's called
on an element which is already as fast as it can be. Especially if we create
txAttributeStep.

Comment 8

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

Hmm.. In findTemplate it would probably be faster to use isXXX functions. At the
most you'll end up with two IsContentOfType calls which should be faster then a
QI. The call in txCopyBase::copyNode seems like it'll have to stay though :(

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky