96478 - AttributeNodes are slow, prefer to do hasAttribute?

Status: VERIFIED FIXED

(Core :: XSLT, defect, P2)

Description

[Axel Hecht]

Another one in the sad tale of AttributeNode in module.

We use the following pattern a few times:

Attr* modeAttr = actionElement->getAttributeNode(MODE_ATTR);
if (modeAttr)
    mode = new String(modeAttr->getValue());
(whitespace cleaned copy ;-) )

Should we move that to

if (actionElement->hasAttribute(MODE_ATTR))
    mode = actionElement->getAttribute(MODE_ATTR);

?

This way we have to lookup the attribute twice, but don't generate an 
AttributeNode with wrappers and foo.

jst, just from the mozilla dom side, is there a speed benefit for either 
solution?

UGH. I just happen to realize, the ownership of AttributeNodes is different
in module and standalone, in module we prolly leak each and every AttributeNode,
but in standalone we mustn't delete them, as we just get references from
the node. yacyacyac.

Occurences:
./source/xslt/ProcessorState.cpp:        Attr* modeAttr =
xslTemplate->getAttributeNode(MODE_ATTR);
./source/xslt/XSLTProcessor.cpp:                Attr* modeAttr =
actionElement->getAttributeNode(MODE_ATTR);
./source/xslt/XSLTProcessor.cpp:                Attr* attr =
actionElement->getAttributeNode(NAME_ATTR);
./source/xslt/XSLTProcessor.cpp:                Attr* attr =
actionElement->getAttributeNode(NAME_ATTR);
./source/xslt/XSLTProcessor.cpp:                Attr* attr =
actionElement->getAttributeNode(NAME_ATTR);
./source/xslt/XSLTProcessor.cpp:    Attr* attr =
xslVariable->getAttributeNode(SELECT_ATTR);

Comment 1

[Axel Hecht]

So I have been taught that we don't leak. Due to nsCOMPtr in 
MozillaObjectWrapper and deleting the wrappers on deleting the doc.

Axel

Comment 2

[Axel Hecht]

we should use getAttr, introduced in nsdoom (bug 76070).

Comment 3

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 4

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

what's left for this bug being marked fixed?

Comment 5

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

the preffered method is |hasAttr(); getAttr();| over |getAttrNode();
getValue();| since the node doesn't contain the actual value. So getValue() will
result in a call to getAttr() behind the scenes.

Patch coming up that removes our last uses of getAttributeNode

Comment 6

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

Attachment: patch to fix

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 71247 [details] [diff] [review]
patch to fix

r=peterv

Comment 8

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

Attachment: remove |getAttribute|

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 71260 [details] [diff] [review]
remove |getAttribute|

r=peterv but i think you should check that replacing isEmpty with the "has
attribute" check is the right thing to do.

Comment 10

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

in most cases "" is not allowed, however there are many other values that are
not allowed since the names should be q-names. So once we start keying stuff on
qnames we should also check for qname-validity. I'll make a note of that in bug
96802.

The only thing that i changed that is not a "qname-attribute" is the "elements"
attribute for xsl:strip-space and xsl:preserve-space. For those elements i would
say it is allowed with "" as attribute value, but the attribute must exist.
Which is what the new code enforces.

(snatching bug since i've got the patch)

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 71247 [details] [diff] [review]
patch to fix

sr=jst

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 71260 [details] [diff] [review]
remove |getAttribute|

sr=jst

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 71247 [details] [diff] [review]
patch to fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 71260 [details] [diff] [review]
remove |getAttribute|

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 15

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

checked in both patches, thanks for reviews

Comment 16

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.