SchemaValidation/Durations have problems with pre-1900 dates due to NSPR

RESOLVED FIXED

Status

RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: doronr, Assigned: doronr)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
x86
All
fixed1.8.0.4, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
 
(Assignee)

Comment 1

13 years ago
Created attachment 214483 [details] [diff] [review]
patch
Attachment #214483 - Flags: review?(aaronr)

Comment 2

13 years ago
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
Attachment #214483 - Flags: review?(aaronr) → review+
(Assignee)

Updated

13 years ago
Blocks: 326556
(Assignee)

Comment 3

13 years ago
fixed on trunk
Status: NEW → ASSIGNED
Whiteboard: xf-to-branch

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 4

13 years ago
checked into 1.8.1
Keywords: fixed1.8.1
(Assignee)

Comment 5

13 years ago
checked into 1.8.0.3
Keywords: fixed1.8.0.3

Updated

13 years ago
Blocks: 332853

Updated

13 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.