Peter, I don't have a testcase. This is based on the conversation we had last week about outstanding XSLT bugs. Please attach testcases if you are aware of them. Thanks! Also, we need more information about what exactly is broken in xsl:number. Should we have separate bugs for each problem? Please make the call.

One thing that I know dosn't work is the "format" attribute.

"the last broken piece in our XSLT support"

Numbering.cpp has a pattern that goes like this: String countAttr; xslNumber->getAttr(txXSLTAtoms::count, kNameSpaceID_None, countAttr); if (!countAttr.isEmpty()) { countPattern = ps->getPattern(xslNumber, ProcessorState::CountAttr); ownsPattern = MB_FALSE; } DIE. This gets the Attr twice, and getPattern returns 0 if the attr didn't exist. If 0 isn't safe enough for you, I can wholeheartly change the signature to nsresult getPattern(Element*, PatternAttr, txPattern*&); in bug 113611. If you do, please say so, 'cause I will probably touch all those lines anyway.

I bumped into this bug, myself. http://www.zvon.org/xxl/XSLTutorial/Books/Output/example21_ch8.html illustrates the expected behavior of xml:number when used with multilevel values. Instead of outputting in an outline format as shown in the example, xml:number returns a sequence along the lines of 1, 11, 12, 2, 21, 22, 23, 3, etc.. It seems to be disregarding the format and level attributes, and just going with the default.

ADT3 per ADT triage.

i've come a fair way on the implementation of this. Unfortuantly i've noticed that the new code isn't much faster then the old one (though of course it actually implements a good deal more of the spec now). So I've got a question for ya: Is the current speed of xsl:number acceptable? A good way to test the speed is to run the numbering tests in buster. I have thought of a way that should improve speed, but it isn't trivial, the question is, is it worth it?

speed is never enough. It might be worth implementing the spec first, get some data from the tests and make a plan for speed then.

This should fully implement xsl:number. There is one thing with the patch that i'm not fully happy with and that is that it uses global static strings, feel free to come with suggestions what to do instead. The patch doesn't implement any i18n counters, that'll have to wait to a later patch. I havn't found any interfaces for getting the data that we need (though i havn't looked too hard) so implementing i18n would probably require some changes on the mozilla side as well

Forgot that i hadn't done the correct thing for numbering documents and attributes (which is compleatly pointless, but nonetheless allowed)

Since Jonas made the fix he should have this bug on his list, reassigning.

Generally, do we need NS_ENSURE_SUCCESS()? List.h/.cpp: please make those functions return nsresult instead of MBool. XSLTProcessor.cpp: @@ -1499,7 +1499,7 @@ // xsl:number else if (localName == txXSLTAtoms::number) { String result; - Numbering::doNumbering(actionElement, result, aPs); + txXSLTNumber::createNumber(actionElement, aPs, result); NS_ASSERTION(mResultHandler, "mResultHandler must not be NULL!"); mResultHandler->characters(result); } Check the return value. txXSLTNumber::createNumber() + if (NS_FAILED(rv)) { + aResult.clear(); + return rv; + } aResult is already cleared. txXSLTNumber::getCounters The code path is a bit funky, if there's no format attr. I'd at least like to see you not call processAttrValueTemplate txXSLTNumber::isAlphaNumeric I wonder if we can share code with XMLUtils here. I think that the counters in txXSLTNumberCounters.cpp could go into txXSLTNumber.cpp. txXSLTNumber::getValueList to come, and txXSLTNumberCounters.cpp too

> Generally, do we need NS_ENSURE_SUCCESS()? Yes, but IMHO that is a separate bug. > XSLTProcessor.cpp: > @@ -1499,7 +1499,7 @@ > // xsl:number > else if (localName == txXSLTAtoms::number) { > String result; > - Numbering::doNumbering(actionElement, result, aPs); > + txXSLTNumber::createNumber(actionElement, aPs, result); > NS_ASSERTION(mResultHandler, "mResultHandler must not be NULL!"); > mResultHandler->characters(result); > } > Check the return value. And do what? The error should already be reported at this stange, and I can't propagate the error any further since it's a void function.

> txXSLTNumber::isAlphaNumeric > I wonder if we can share code with XMLUtils here. Just looked into this and unfortuantly we can't really share much. The only thing I could call into from txXSLTNumber::isAlphaNumeric is XMLUtils::isDigit, which probably gives more pain then the 15 lines of gain.

Re: #12, just add a comment then, so once we fix it, we know that we have to fix that there. re NS_ENSURE_SUCCESS, sneaking in that macro is less hassle than doing all those if (NS_FAILED(rv)) now and fix them later. Re: #13, so leave that code like it is, bad spec to have all kinds of char defs around.

txXSLTNumber::getValueList if value < 0.5, shouldn't you output 0, instead of Double::toString(value, aValueString) ? I wonder if we should use atoms for level, and compare against txXSLTAtoms::multiple etc. You don't error on level="junk". Index: source/xslt/txXSLTNumber.h +class txFormattedCounter; not needed. txDecimalCounter::txDecimalCounter why do you set this mGroupSize to 50? txDecimalCounter::appendNumber + // in case we didn't get a long enough string + while (pos > 0 && bufsize - pos < mMinLength) { + buf[--pos] = '0'; + } make this a for loop. + // in case we *still* didn't get a long enough string + // this should be very rare + PRInt32 extraPos = mMinLength; + while (extraPos > bufsize - pos) { pos is 0 here, and adjust the comment to say that mMinLength is bigger than the length of any PRInt32, so we have to pad that here. I'm thru with it.

> Re: #12, just add a comment then, so once we fix it, we know that we have to > fix that there. Done > re NS_ENSURE_SUCCESS, sneaking in that macro is less hassle than doing all thos > if (NS_FAILED(rv)) > now and fix them later. Done, I added NS_ENSURE_TRUE and NS_ENSURE_FALSE too > txXSLTNumber::getValueList > if value < 0.5, shouldn't you output 0, instead of > Double::toString(value, aValueString) ? Nope, look in the errata. I actually don't do exactly as the errata says, but rather do what XSLT2 says which is sligtly different, it says that the head and tail of the format should be added even in this case. > I wonder if we should use atoms for level, and compare against > txXSLTAtoms::multiple etc. You don't error on level="junk". Done > +class txFormattedCounter; > not needed. Done > txDecimalCounter::txDecimalCounter > why do you set this mGroupSize to 50? This guarantees that the grouping separator will never be inserted, i.e. we use a plain decimal counter. This ctor is only used in txRomanCounter::appendNumber for numbers > 3999 where roman counting isn't defined. > + // in case we didn't get a long enough string > + while (pos > 0 && bufsize - pos < mMinLength) { > + buf[--pos] = '0'; > + } > make this a for loop. The only two for-loops i can make of this is for (; pos > 0 && bufsize - pos < mMinLength; buf[--pos] = '0'); (which will give a warning) and for (; pos > 0 && bufsize - pos < mMinLength; buf[pos] = '0') { --pos; } which i both think are worse then the while-loop > + // in case we *still* didn't get a long enough string > + // this should be very rare > + PRInt32 extraPos = mMinLength; > + while (extraPos > bufsize - pos) { > pos is 0 here, and adjust the comment to say that mMinLength is bigger than > the length of any PRInt32, so we have to pad that here. Done

Addresses Axels comments. Oh, the reason i split up the counters into thier own file is when we do i18n we will probably get quite a lot of ugly lengthy code which is nice to keep separate from everything else. Also fixed the mGroupSize = 50, Axel is indeed right

Comment on attachment 95376 [details] [diff] [review] Patch to fully implement xsl:number v2 marking needs work, thought this is much more "needs -N". Most of the code is missing :-(

oops, cvs add is good

Comment on attachment 95503 [details] [diff] [review] Patch to fully implement xsl:number v2 to build this on standalone, I had to switch from private to protected in TxString.h:188, and change PRBool to MBool in txXSLTNumberCounters.cpp:74 and add #include "txError.h" to List.h my version of the loop would be PRInt32 end = (bufsize > mMinLength) ? bufsize - mMinLength : 0; while (pos > end) { vs. while (pos > 0 && bufsize - pos < mMinLength) { looking at functionality now

Comment on attachment 95503 [details] [diff] [review] Patch to fully implement xsl:number v2 r=axel@pike.org, as my comments are addressed. We had some thoughts about what we should do for empty value lists, as Xalan seems to take a little freedom and some 0s there. Jonas's patch does what saxon does, so it's the spec and Jonas and saxon and I vs. Xalan. Sorry buddy. Oh, IE seems to be on the specs side, too. We'll adjust the xalan tests, I guess.

Fixes the buildproblems and the while-loop

Comment on attachment 95548 [details] [diff] [review] Patch to fully implement xsl:number v2.1 carrying over r=Pike

Fixes a couple of stringchanges peterv requested

Comment on attachment 95898 [details] [diff] [review] Patch to fully implement xsl:number v2.2 carrying review

Comment on attachment 95898 [details] [diff] [review] Patch to fully implement xsl:number v2.2 - In txError.h: +#define NS_ENSURE_TRUE(value, result) \ + if (!(value)) { \ + return (result); \ + } Please use the PR_BEGIN_MACRO and PR_END_MACRO macro's here to avoid having the side-effects of the "if" in the macro leak outside the macro. (by doing this an else after one of those macro's won't do the unexpected and the compiler will require a ; after those macros). Other than that, sr=jst

Comment on attachment 95898 [details] [diff] [review] Patch to fully implement xsl:number v2.2 - In txError.h: +#define NS_ENSURE_TRUE(value, result) \ + if (!(value)) { \ + return (result); \ + } Please use the PR_BEGIN_MACRO and PR_END_MACRO macro's here to avoid having the side-effects of the "if" in the macro leak outside the macro. (by doing this an else after one of those macro's won't do the unexpected and the compiler will require a ; after those macros). Other than that, sr=jst

addresses jsts comments and merges to tip

checked in! thanks for reviews

mass verifying