96802 - (qname) Key stuff on expanded name, not QName

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

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

Some things in a stylesheet should be keyed on expanded name rather then just 
it's string. Section 2.4 of the spec summs this up pretty well (see url).

The following things should key on expanded name instead of string:
* Named templates
* Template modes
* Named attribute sets
* Keys
* Decimal formats
* Variables/parameters

One thing to note about this is that the key() function gets it's qname at 
runtime, so it has to resolve qname->expanded name at runtime... :(


Atually this shouldn't be that hard to implement once bug 76070 is checked in. 
All we need is a class that is a tuple consisting of a namespace-id and a 
localname-atom and then use that as key in some Map-like class.

In fact if we made a class like this:

class txExpandedName : public TxObject {
    txExpandedName(String& aQName, Node* aResolver) {
        ...
    }

    PRInt32 hashCode() {
        return NS_PTR_TO_INT32(mLocalName) + mNsID;
    }
    MBool equals(TxObject* obj) {
        txExpandedName* other = (txExpandedName*)obj;
        return other->mNsID == mNsID && other->mLocalName = mLocalName;
    }

    PRInt32 mNsID;
    txAtom* mLocalName;
};

we could even use our current Map class!

Comment 1

[Axel Hecht]

Hrm. Recap:
*slap*

the first argument of key() is a string. So we can do that at parse time.
(as opposed to evaluation time, if I get you right).

I'm all for a special struct to hold (nsID,localNameAtom) tuples. We have a 
fixed size data struct, so put that into an array, and be done.

The use-cases don't have alot of instances, in general, so hash might be too
much (and too little ;-))

Template foo is probably special.

How about a struct, and a macro for compares? This stuff should be inlined,
really.

Axel

Comment 2

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

Nope, key() must be done at runtime, this is perfectly valid expression:

"key(@type, bar)"

It's just that you take the string-value of "@type" as keyname. The advantage 
of the exsiting Map class is that it has a very convenient interface and it is 
already implemented.

As you say, there's not really that big of speed issue here since we won't 
stick too much stuff in there so the fact that Map currently is damn slow is 
not really a problem (and we should really make Map fast some day).

But hey, if you wanna make a list-based implementation of a Map-like interface 
I'll be happy to review it ;-)

Comment 3

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

i might as well take this. This is a pretty easy fix once the new variables 
code is in since we'll have a txExpandedNameMap class then.

Comment 4

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

note to self: make sure that we throw error for required attributes that are
missing and for attributes that are not valid qnames.

Comment 5

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

Attachment: patch to fix everything but variables

The patch fixes everything except variables which i'll do in a separate patch.
The patch is pretty straight forward and follows the approx same pattern
everywhere.

The only real logic i had to change was for decimal formats. The old code used
to add the default-format to the decimal-formats-map in ProcessorState::init
and then remove from the map if the default format was explicitly set. However
since the new map can't handle removing I now never add the default-format to
the map and instead get the default manually in
ProcessorState::getDecimalFormat.
Hmm.. come to think of it.. i could use txExpandedNameMap::set to change the
default format.. but i kind'a like the new way better. Let me know if you don't
though.

Comment 6

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

This needs this too:


Index: source/base/txExpandedNameMap.cpp
===================================================================
RCS 
file: /cvsroot/mozilla/extensions/transformiix/source/base/txExpandedNameMap.cpp
,v
retrieving revision 1.1
diff -u -r1.1 txExpandedNameMap.cpp
--- source/base/txExpandedNameMap.cpp	2 Mar 2002 09:35:20 -0000	1.1
+++ source/base/txExpandedNameMap.cpp	3 Mar 2002 22:45:36 -0000
@@ -88,6 +88,7 @@
     
     mItems[mItemCount].mNamespaceID = aKey.mNamespaceID;
     mItems[mItemCount].mLocalName = aKey.mLocalName;
+    TX_IF_ADDREF_ATOM(aKey.mLocalName);
     mItems[mItemCount].mValue = aValue;
     ++mItemCount;
     
@@ -129,6 +130,7 @@
     
     mItems[mItemCount].mNamespaceID = aKey.mNamespaceID;
     mItems[mItemCount].mLocalName = aKey.mLocalName;
+    TX_IF_ADDREF_ATOM(aKey.mLocalName);
     mItems[mItemCount].mValue = aValue;
     ++mItemCount;
     

Comment 7

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

pushing

Comment 8

[Axel Hecht]

stuff while reading the patch:
There is a 
if(NS_FAILED(rv))

I would prefer to have nsresult rv = NS_OK; at least at those places where
a if () clause makes the first assignment conditional.

"Bad attribute" is a bad error message, we should come up with something better.
"Required attribute %s is missing or malformed"?
I'm not sure if we want a separate error for required attributes, though.

Comment 9

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

Attachment: address Axels comments

Comment 10

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

pushing :(

Comment 11

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

...

Comment 12

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

Attachment: sync with tip

syncs with tip and fixes a small (unrelated) bug in
ProcessorState::addLREStylesheet since i was fiddling around there anyway.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: resync with tip

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 90670 [details] [diff] [review]
resync with tip

>Index: source/xslt/ProcessorState.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/ProcessorState.cpp,v
>retrieving revision 1.64
>diff -u -3 -r1.64 ProcessorState.cpp
>--- source/xslt/ProcessorState.cpp	2 Jul 2002 14:12:58 -0000	1.64
>+++ source/xslt/ProcessorState.cpp	9 Jul 2002 21:54:18 -0000

>@@ -866,26 +891,19 @@
...
>     txDecimalFormat* existing = NULL;
...
>+    existing = (txDecimalFormat*)mDecimalFormats.get(formatName);

Please make those two lines into one.

r=peterv.

I'll attach a diff against a tree with your patch with some additional changes
I propose.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Proposal for more qnames

Comment 16

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

comment 14 done in local tree

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 90670 [details] [diff] [review]
resync with tip

sr=jst

Comment 18

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

Comment on attachment 90670 [details] [diff] [review]
resync with tip

checked in

Comment 19

[Axel Hecht]

I read rumors about ExprLexer and QNames in #transformiix, so here what I
intented back in the days when that thing had it's rewrite:
Tokens should hold iterators to the original string, thus providing position
information. Which is still kinda cheap, too. I'd say, don't get atoms until we 
need to.

Comment 20

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

fixed on branch with the checkin of bug 117658

Comment 21

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

fixed with the branchlanding.

Comment 22

[Axel Hecht]

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