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!

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

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 ;-)

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.

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

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.

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;

pushing

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.

pushing :(

...

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

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 14 done in local tree

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

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

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.

fixed on branch with the checkin of bug 117658

fixed with the branchlanding.

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