Comment on attachment 214483 [details] [diff] [review] patch Please use -p in your cvs diffs so it is easier to figure out the functions where your changes are. Another general comment that is widespread in this patch -> why not just have one function like this: static PRBoolIsValidSchemaInteger(const nsAString & aNodeValue, long *aResult, PRBool aOverFlowCheck = PR_FALSE); Then you don't have to go through a forwarder when it only has two parameters and you pass in the PR_FALSE in explicitly. Another general comment: I don't see where you test for aNodeValue existing or being a string of any length at all when you are checking to see if aNodeValue is a valid type of something. For most of these couldn't you bow out early if aNodeValue wasn't there or of 0 length? >Index: extensions/schema-validation/src/nsSchemaValidator.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v >retrieving revision 1.20 >diff -u -r1.20 nsSchemaValidator.cpp >--- extensions/schema-validation/src/nsSchemaValidator.cpp 21 Feb 2006 22:01:13 -0000 1.20 >+++ extensions/schema-validation/src/nsSchemaValidator.cpp 8 Mar 2006 23:10:42 -0000 >@@ -2081,26 +2084,17 @@ > PRBool *aResult) > { > PRBool isValid = PR_FALSE; >- PRTime dateTime; >- PRExplodedTime explodedTime; >+ nsSchemaTime time; > int timeCompare; > >- isValid = IsValidSchemaTime(aNodeValue, &dateTime); >- >- if (isValid) { >- // convert it to a PRExplodedTime >- PR_ExplodeTime(dateTime, PR_GMTParameters, &explodedTime); >- } >+ isValid = IsValidSchemaTime(aNodeValue, &time); Why not just declare isValid to be a PRBool here instead of assigning it PR_FALSE to begin with? >@@ -721,14 +693,98 @@ > } > > void >-nsSchemaValidatorUtils::GetMonthShorthand(char* aMonth, nsACString & aReturn) >+nsSchemaValidatorUtils::AddTimeZoneToDateTime(nsSchemaDateTime aDateTime, >+ nsSchemaDateTime* aDestDateTime) > { >+ >+ printf("\n ::: %u:%d:%d %d:%d:%d\n", year, month, day, hour, minute, second); >+ nit: make printf only during debug or surround with LOG. > PRBool done = PR_FALSE; > while (!done) { >- maxDay = GetMaximumDayInMonthFor(resultDatetime.tm_year, >- resultDatetime.tm_month); >- if (resultDatetime.tm_mday < 1) { >- resultDatetime.tm_mday += >- GetMaximumDayInMonthFor(resultDatetime.tm_year, >- resultDatetime.tm_month - 1); >+ maxDay = GetMaximumDayInMonthFor(aResultDateTime->date.year, >+ aResultDateTime->date.month); nit: align parameters, please. >Index: extensions/schema-validation/src/nsSchemaValidatorUtils.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidatorUtils.h,v >retrieving revision 1.7 >diff -u -r1.7 nsSchemaValidatorUtils.h >--- extensions/schema-validation/src/nsSchemaValidatorUtils.h 21 Feb 2006 22:01:13 -0000 1.7 >+++ extensions/schema-validation/src/nsSchemaValidatorUtils.h 8 Mar 2006 23:10:43 -0000 >- static PRBool GetPRTimeFromDateTime(const nsAString & aNodeValue, PRTime *aResult); >+ static int CompareDateTime(nsSchemaDateTime aDateTime1, >+ nsSchemaDateTime aDateTime2); nit: align parameters, please. with these, r=me

fixed on trunk

checked into 1.8.1

checked into 1.8.0.3