Finish out XPath extension functions

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
13 years ago
a year ago

People

(Reporter: aaronr, Assigned: aaronr)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 8 obsolete attachments)

1.31 KB, application/xhtml+xml
Details
1.21 KB, application/xhtml+xml
Details
1.26 KB, application/xhtml+xml
Details
1.18 KB, application/xhtml+xml
Details
9.40 KB, patch
sicking
: review+
Details | Diff | Splinter Review
19.66 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
We still need to support index(), days-from-date(), months(), seconds(), and
seconds-from-dateTime().  The last four are blocked on 263384.  Doron has the
utility functions in that patch that I need to finish the work.
(Assignee)

Updated

13 years ago
Depends on: 263384

Comment 1

13 years ago
(In reply to comment #0)
> We still need to support index(), days-from-date(), months(), seconds(), and
> seconds-from-dateTime().  The last four are blocked on 263384.

And the first one on 278209.
Depends on: 278209
(Assignee)

Comment 2

13 years ago
I'm implementing index() under 282777
Status: NEW → ASSIGNED
Depends on: 282777
No longer depends on: 278209
(Assignee)

Updated

13 years ago
Depends on: 223097
No longer depends on: 263384
(Assignee)

Comment 3

13 years ago
Created attachment 182937 [details] [diff] [review]
adds remaining functions

This patch is dependent on the patch for bug 223097 to work.  But assuming that
fix is in, this patch should provide all the necessary functionality for
seconds(), seconds-from-dateTime(), months(), and days-from-date().
(Assignee)

Comment 4

13 years ago
Created attachment 187707 [details]
testcase for seconds-from-dateTime
(Assignee)

Comment 5

13 years ago
Created attachment 187708 [details]
testcase for seconds
(Assignee)

Comment 6

13 years ago
Created attachment 187709 [details]
testcase for days-from-date
(Assignee)

Comment 7

13 years ago
Created attachment 187710 [details]
testcase for months
(Assignee)

Comment 8

13 years ago
Created attachment 187712 [details] [diff] [review]
ready for review

ready for review now that pre-reqs are in
Attachment #182937 - Attachment is obsolete: true
Attachment #187712 - Flags: review?(doronr)

Comment 9

13 years ago
Comment on attachment 187712 [details] [diff] [review]
ready for review

Review for the extensions/xforms/ part, you'll need an sr for the transformixx
parts.

>Index: extensions/xforms/nsXFormsUtilityService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.cpp,v
>retrieving revision 1.5
>diff -u -8 -p -r1.5 nsXFormsUtilityService.cpp
>--- extensions/xforms/nsXFormsUtilityService.cpp	29 Jun 2005 18:14:43 -0000	1.5
>+++ extensions/xforms/nsXFormsUtilityService.cpp	29 Jun 2005 21:51:49 -0000
...
> NS_IMETHODIMP 
> nsXFormsUtilityService::ValidateString(const nsAString & aValue, 
>-                                         const nsAString & aType, 
>-                                         const nsAString & aNamespace,
>-                                         PRBool *aResult)
>+                                       const nsAString & aType, 
>+                                       const nsAString & aNamespace,
>+                                       PRBool *aResult)
> {
> 
>-  // XXX TODO This function needs to call the XForms validator layer from
>-  //   bug 274083 when it goes into the build.
>+  NS_ASSERTION(aResult, "no return buffer for result so we'll crash soon");
> 
>-#if 0
>-  nsresult rv = NS_ERROR_FAILURE;
>   nsXFormsSchemaValidator *validator = new nsXFormsSchemaValidator();
>   *aResult = validator->ValidateString(aValue, aType, aNamespace);
>-  return rv;
>-#endif

Any need to make sure nsXFormsSchemaValidator() doesn't return null?

>+      nsAutoString totalSeconds;
>+      totalSeconds.AppendInt(sumSeconds);
>+      
>+      if (fractionalSeconds) {
>+        nsAutoString fraction;
>+        fraction.AppendInt(fractionalSeconds);
>+        totalSeconds.Append(NS_LITERAL_STRING("."));
>+        totalSeconds.Append(fraction);
>+      }

AppendLiteral(".") would be cleaner.

>+        totalSeconds.Append(NS_LITERAL_STRING("."));
>+        totalSeconds.Append(fraction);
>+      }

Ditto

>+    rv = schemaValidator->ValidateBuiltinTypeDate(dateString, &date); 
>+    if (!NS_SUCCEEDED(rv)) {
>+      // if it is a dateTime, we need to ignore hours, minutes and seconds
>+    }

Um, that does nothing....
Attachment #187712 - Flags: review?(doronr) → review+
(Assignee)

Comment 10

13 years ago
Created attachment 187826 [details] [diff] [review]
fixed comments

peterv, could you do the sr since it involves changes to transformiix?	Changes
only in the XForms specific code, though.
Attachment #187712 - Attachment is obsolete: true
Attachment #187826 - Flags: superreview?(peterv)
(Assignee)

Comment 11

13 years ago
Comment on attachment 187826 [details] [diff] [review]
fixed comments

Jonas, can you review?	peterv is tough to catch.  I'll get tor or someone to
sr.
Attachment #187826 - Flags: superreview?(peterv) → review?(bugmail)
Comment on attachment 187826 [details] [diff] [review]
fixed comments

>Index: extensions/transformiix/source/xpath/XFormsFunctionCall.cpp
>@@ -146,17 +146,31 @@ XFormsFunctionCall::evaluate(txIEvalCont
>    
>       return aContext->recycler()->getNumberResult(count, aResult);
>     }
>     case DAYSFROMDATE:
>     {
>       if (!requireParams(1, 1, aContext))
>         return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>    
>-      return NS_ERROR_NOT_IMPLEMENTED;
>+      nsAutoString date;
>+      evaluateToString((Expr*)iter.next(), aContext, date);
>+   
>+      nsCOMPtr<nsIXFormsUtilityService>xformsService = 
>+            do_GetService("@mozilla.org/xforms-utility-service;1", &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      PRInt32 result = 0;
>+      double res = Double::NaN;
>+      nsresult rv = xformsService->GetDaysFromDateTime(date, &result);
>+      if (NS_SUCCEEDED(rv)) {
>+        res = result;
>+      }

You probably don't want to return NaN for *all* errors. Ideally you should
throw some specific error when the conversion fail and just recover from that
error by returning NaN. Other error codes would be forwarded up through the
callchain.

Same for the other places in this class.

>Index: extensions/xforms/nsXFormsUtilityService.cpp

> nsXFormsUtilityService::ValidateString(const nsAString & aValue, 
>-                                         const nsAString & aType, 
>-                                         const nsAString & aNamespace,
>-                                         PRBool *aResult)
>+                                       const nsAString & aType, 
>+                                       const nsAString & aNamespace,
>+                                       PRBool *aResult)
> {
> 
>-  // XXX TODO This function needs to call the XForms validator layer from
>-  //   bug 274083 when it goes into the build.
>+  NS_ASSERTION(aResult, "no return buffer for result so we'll crash soon");

Not sure the word 'buffer' is entierly right there... You do that in other
places too. IMHO you don't need this assertion at all, but that's up to you.

>+  *aResult = PR_FALSE;
> 
>-#if 0
>-  nsresult rv = NS_ERROR_FAILURE;
>   nsXFormsSchemaValidator *validator = new nsXFormsSchemaValidator();
>-  *aResult = validator->ValidateString(aValue, aType, aNamespace);
>-  return rv;
>-#endif
>-
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  if (validator) {
>+    *aResult = validator->ValidateString(aValue, aType, aNamespace);
>+  }
>+  return *aResult ? NS_OK : NS_ERROR_FAILURE;

This looks like it'll leak an nsXFormsSchemaValidator on every call.


And now for some style nits :)

You should read

http://www.cas.mcmaster.ca/~carette/SE3M04/2004/slides/mozilla-style-guide.html
#Errors

>+NS_IMETHODIMP
>+nsXFormsUtilityService::GetMonths(const nsAString & aValue, 
>+                                  PRInt32         * aMonths)
>+{
>+  NS_ASSERTION(aMonths, "no return buffer for months, we'll crash soon");
>+  nsresult rv = NS_ERROR_FAILURE;
>+
>+  *aMonths = 0;
>+  nsCOMPtr<nsISchemaDuration> duration;
>+  nsCOMPtr<nsISchemaValidator> schemaValidator = 
>+    do_GetService(NS_SCHEMAVALIDATOR_CONTRACTID);
>+  if (schemaValidator) {

Put an NS_ENSURE_TRUE here and remove the |if|.

>+    schemaValidator->ValidateBuiltinTypeDuration(aValue, 
>+                                                 getter_AddRefs(duration));
>+    if (duration) {

Same here

>+      PRInt32 sumMonths;
>+      PRUint32 years;
>+      PRUint32 months;
>+
>+      duration->GetYears(&years);
>+      duration->GetMonths(&months);
>+
>+      sumMonths = months + years*12;
>+      PRBool negative;
>+      duration->GetNegative(&negative);
>+      if (negative) {
>+        // according to the spec, "the sign of the result will match the sign
>+        // of the duration"
>+        sumMonths *= -1;
>+      }
>+      
>+      *aMonths = sumMonths;
>+      rv = NS_OK;
>+    }
>+  }
>+
>+  return rv;

And then |return NS_OK;| here.

I've run into some very weird errors because people insist on doing |return rv|
at the end of a function. This gets very dangerous if you add some code that
uses rv but can fallback from errors (like your NaN returns above).

Always check for errors early and return NS_OK at the end. This makes for
cleaner code and fewer bugs.

Same goes for all the new functions below in this class.

In general, something is usually (though not always) written the wrong way when
you're using

if (NS_SUCCEEDED(rv)) {
   ...

9 times out of 10 that should have been an NS_ENSURE_SUCCESS or |if
(NS_FAILED(rv))| with an extra codepath for handling an error rather then an
|if| handling a success.

>+NS_IMETHODIMP
>+nsXFormsUtilityService::GetSeconds(const nsAString & aValue, 
>+                                   nsAString       & aSeconds)
>+{
>+  nsresult rv = NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsISchemaDuration> duration;
>+  nsCOMPtr<nsISchemaValidator> schemaValidator = 
>+    do_GetService(NS_SCHEMAVALIDATOR_CONTRACTID);
>+  if (schemaValidator) {
>+    schemaValidator->ValidateBuiltinTypeDuration(aValue, 
>+                                                 getter_AddRefs(duration));
>+    if (duration) {
>+      PRUint32 sumSeconds;
>+      PRUint32 days;
>+      PRUint32 hours;
>+      PRUint32 minutes;
>+      PRUint32 seconds;
>+      PRUint32 fractionalSeconds;
>+
>+      duration->GetDays(&days);
>+      duration->GetHours(&hours);
>+      duration->GetMinutes(&minutes);
>+      duration->GetSeconds(&seconds);
>+      duration->GetFractionSeconds(&fractionalSeconds);

Shouldn't this use months and years too? If so, you might want to use something
bigger then an PRUint32 when you add it all up.

Actually, why not make GetSeconds and GetSecondsFromDateTime return a double
rather then a string. Seems like a waste of effort to convert
number->string->number
Attachment #187826 - Flags: review?(bugmail) → review-
Actually, the entire fractionSeconds field in duration seems very wrongly
implemented. Won't it parse 0.01 and 0.1 into the same value (i.e. a fraction of
1). Seems like it should be stored as a double rather then an int.
(In reply to comment #13)
> Actually, the entire fractionSeconds field in duration seems very wrongly
> implemented. Won't it parse 0.01 and 0.1 into the same value (i.e. a fraction of
> 1). Seems like it should be stored as a double rather then an int.

That is being worked on.  Schema validation has issues with fraction seconds
(nspr won't parse fractionseconds in datetimes for example) which I am working on.
(Assignee)

Comment 15

13 years ago
Created attachment 189095 [details] [diff] [review]
fixed review comments

fixed all the review comments except those about my choice of the word 'buffer'
in my assertions.  I looked through the thesaurus for other suggestions, but no
others were appealing :=)
Attachment #187826 - Attachment is obsolete: true
Attachment #189095 - Flags: review?(bugmail)
Comment on attachment 189095 [details] [diff] [review]
fixed review comments

>Index: extensions/transformiix/source/xpath/XFormsFunctionCall.cpp
>+      PRInt32 result = 0;
>+      double res = Double::NaN;
>+      nsresult rv = xformsService->GetDaysFromDateTime(date, &result);
>+      if (NS_SUCCEEDED(rv)) {
>+        res = result;
>+      } else if (rv != NS_ERROR_ILLEGAL_VALUE) {
>+        // if we failed for a reason other than the parameter value, pass that 

Please follow the style in the rest of the file (and in the rest of
transformiix)

}
else if (...) {

Same for MONTHS

In SECONDS and SECONDSFROMDATETIME

>+      double res = Double::NaN;

No need to set an initial value here.

>+      nsresult rv = xformsService->GetSeconds(duration, &res);
>+      if (!NS_SUCCEEDED(rv)) {

NS_FAILED(rv)

>+        if (rv != NS_ERROR_ILLEGAL_VALUE) {
>+          // if we failed for a reason other than the parameter value, pass that 
>+          // up the chain
>+          return rv;
>+        } else {
>+          res = Double::NaN;

Remove else-after-return



>Index: extensions/xforms/nsXFormsUtilityService.cpp
...
>+NS_IMETHODIMP
>+nsXFormsUtilityService::GetMonths(const nsAString & aValue, 
>+                                  PRInt32         * aMonths)
>+{
>+  NS_ASSERTION(aMonths, "no return buffer for months, we'll crash soon");
>+  nsresult rv = NS_ERROR_FAILURE;
>+
>+  *aMonths = 0;
>+  nsCOMPtr<nsISchemaDuration> duration;
>+  nsCOMPtr<nsISchemaValidator> schemaValidator = 
>+    do_GetService(NS_SCHEMAVALIDATOR_CONTRACTID);
>+  NS_ENSURE_TRUE(schemaValidator, rv);

Don't rely on old values of rv. Either do

nsCOMPtr<nsISchemaValidator> schemaValidator = 
  do_GetService(NS_SCHEMAVALIDATOR_CONTRACTID, rv);
NS_ENSURE_SUCCESS(rv, rv);

or

nsCOMPtr<nsISchemaValidator> schemaValidator = 
  do_GetService(NS_SCHEMAVALIDATOR_CONTRACTID);
NS_ENSURE_TRUE(schemaValidator, NS_ERROR_FAILURE);

This makes it clear what happens and doesn't break if someone uses rv above
your code.

>+  rv = schemaValidator->ValidateBuiltinTypeDuration(aValue, 
>+                                                    getter_AddRefs(duration));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_TRUE(duration, NS_ERROR_FAILURE);

Both these shouldn't be needed, either one should be enough. The
NS_ENSURE_SUCCESS might be nicer since it'll propagate the right errorcode.

Same in the remaining functions.

>+  sumSeconds = seconds + minutes*60 + hours*3600 + days*24*3600;
>+  PRBool negative;
>+  duration->GetNegative(&negative);
>+  if (negative) {
>+    // according to the spec, "the sign of the result will match the sign
>+    // of the duration"
>+    sumSeconds *= -1;
>+  }

This should be done after fractionalSeconds, no?

>+  if (fractionalSeconds) {
>+    nsAutoString fraction;
>+    fraction.AppendInt(fractionalSeconds);
>+    double convertedFraction = fractionalSeconds/(pow(10, fraction.Length()));
>+    sumSeconds += convertedFraction;
>+  }

Personally I would just skip fractionalSeconds for now since they seem totally
borked. Up to you.

with that, r=me
Attachment #189095 - Flags: review?(bugmail) → review+
(Assignee)

Comment 17

13 years ago
Created attachment 189217 [details] [diff] [review]
fixed latest comments

tor, could you sr, please?
Attachment #189095 - Attachment is obsolete: true
Attachment #189217 - Flags: superreview?(tor)
You didn't add years or months to GetSeconds. I take it it's not needed? Would
make sense since a month isn't a set number of days (lousy Julius Caesar).
(Assignee)

Comment 19

13 years ago
(In reply to comment #18)
> You didn't add years or months to GetSeconds. I take it it's not needed? Would
> make sense since a month isn't a set number of days (lousy Julius Caesar).

Yeah, and a year isn't necessarily 365 days, either.

Sorry, I meant to mention that in a previous comment and forgot.  The XForms
XPath seconds() function says to ignore months and years.  In section 7.10.4 of
the spec, "Year and month components, if present, are ignored"
Lousy earth orbit

Comment 21

13 years ago
Lousy calendar system.

Comment 22

13 years ago
Comment on attachment 189217 [details] [diff] [review]
fixed latest comments

Fractional seconds as currently implemented are totally borked (see comment
13).
Attachment #189217 - Flags: superreview?(tor) → superreview-
(Assignee)

Comment 23

13 years ago
(In reply to comment #22)
> (From update of attachment 189217 [details] [diff] [review] [edit])
> Fractional seconds as currently implemented are totally borked (see comment
> 13).
> 

Sure, I'll give the wrong answer if someone passes me a duration that contains
fractional seconds, but once the bug is fixed, then this code will work just
fine with no further changes.  If I ignore factional seconds, then I'll still
give the wrong answer since I won't take into account the fractional seconds in
the duration.  And then it'll take another patch to fix it so that the code
ISN'T igonoring the fractional seconds when the appropriate patch is made in
schema-validation or NSPR.

What change do I need to do to this patch to get it in?
Actually, the only way to fix fractional seconds is to make it be stored as a
double. So then you will need to change the current code anyway (to just add the
double). Durations are currently borked in such a way that it is impossible to
write the code the right way, which is why i suggested to just ignore fractions
for now.
Created attachment 189764 [details] [diff] [review]
make schema duration return doubles
(Assignee)

Comment 26

13 years ago
(In reply to comment #25)
> Created an attachment (id=189764) [edit]
> make schema duration return doubles
> 

this change looks ok to me.
Comment on attachment 189764 [details] [diff] [review]
make schema duration return doubles

asking for sr :)
Attachment #189764 - Flags: superreview?(peterv)
(Assignee)

Comment 28

13 years ago
Comment on attachment 189764 [details] [diff] [review]
make schema duration return doubles

Jonas, can you do a review of this duration change so that we can get this in
and I can then get my xpath extension stuff changed and put in also?
Attachment #189764 - Flags: review?(bugmail)
Comment on attachment 189764 [details] [diff] [review]
make schema duration return doubles

Why parse the integer and fraction part separatly? Just parse them together and
then separate them.

Use PR_strtod since I suspect that strtod will give you localization trouble.

Also,
strlen(p) == 0
is just a very slow version of
*p == 0.

And you might want to assert that aString isn't null.
Attachment #189764 - Flags: review?(bugmail) → review-
(In reply to comment #29)
> (From update of attachment 189764 [details] [diff] [review] [edit])
> Why parse the integer and fraction part separatly? Just parse them together and
> then separate them.
> 

If I had one string and had to separate it wouldn't that be slower (an
additional walk of the string to find '.')?  And storting the offset somewhere
for where the fractional part started seems ugly.
The parsing code is pretty slow anyway, and it might not neccesarily have to do
an additional walk to find the '.'. And in any case it's faster then
concatinating "0." and the fraction part.
Additionally, without parsing integer and fraction together you won't be able to
get a best-effort result for something like '1.999999999999999999999'  (should
be stored as 2)
Created attachment 190468 [details] [diff] [review]
schema duration, try 2
Attachment #189764 - Attachment is obsolete: true
Attachment #190468 - Flags: review?(bugmail)
I kept the logic as is and added a check to not clear the buffer when we are
passing fraction seconds.  The reason is to check for say 34.3.3.  Could have
used  CountChar perhaps, but that sounded like overkill since we already iterate
over all characters.  I could have checked the buffer if it an valid double and
then gotten the int and fraction parts from the double, but that could lead to
rounding fun.
Comment on attachment 190468 [details] [diff] [review]
schema duration, try 2

You *want* the rounding fun. As things stand now if you parse '1.9999999...'
you'll get 1 as integer part and 1.0 as fraction. What you want is 2 as integer
and 0 as fraction.

What you do there is actually slower then just doing a single PR_strtod. It
does not have to search for the '.'. And it wouldn't matter if it did, check
the code, scanning is nothing.

Also, if you cared about performance you should use flatstrings, not nsAString,
when iterating. That way you could avoid stringcopies compleatly, and the
iterator would be much faster.

I don't quite understand the problem with "34.3.3". Shouldn't that cause you to
note that there are two decimal-points (|if (fractionSecondFound){done=true}|)
and bail?


Don't make me come over there!
Attachment #190468 - Flags: review?(bugmail) → review-
Created attachment 190561 [details] [diff] [review]
per sicking's comments
Attachment #190468 - Attachment is obsolete: true
Attachment #190561 - Flags: review?(bugmail)
Comment on attachment 190561 [details] [diff] [review]
per sicking's comments

>               } else {
>-                second = temp;
>-                fractionSecond = temp2;
>+                fractionSecond = modf(temp2, &intpart);
>+                second = (int) intpart;

I think you might want to use an NS_STATIC_CAST here. And cast to PRUint32
rather then int.

>Index: extensions/schema-validation/src/nsSchemaValidatorUtils.h
...
>+  static PRBool IsValidSchemaDouble(const nsAString & aNodeValue, double *aResult);
>+  static PRBool IsValidSchemaDouble(const char* aString, double *aResult);

You probably shouldn't expose these methods publicly since you don't actually
enforce that it is a valid schema-double (PR_strtod supports more chars then
you want and you don't check that the entire string got parsed).

with that fixed, r=me.
Attachment #190561 - Flags: review?(bugmail) → review+
Oh, and if you want performance you should make parseBuffer an ns*C*AutoString
to avoid copying that string twice.
(In reply to comment #37)
> (From update of attachment 190561 [details] [diff] [review] [edit])
> >               } else {
> >-                second = temp;
> >-                fractionSecond = temp2;
> >+                fractionSecond = modf(temp2, &intpart);
> >+                second = (int) intpart;
> 
> I think you might want to use an NS_STATIC_CAST here. And cast to PRUint32
> rather then int.
> 
> >Index: extensions/schema-validation/src/nsSchemaValidatorUtils.h
> ...
> >+  static PRBool IsValidSchemaDouble(const nsAString & aNodeValue, double
*aResult);
> >+  static PRBool IsValidSchemaDouble(const char* aString, double *aResult);
> 
> You probably shouldn't expose these methods publicly since you don't actually
> enforce that it is a valid schema-double (PR_strtod supports more chars then
> you want and you don't check that the entire string got parsed).
> 
> with that fixed, r=me.
> 

Doesn't the return (pEnd != aString) check if the whole string was parsed?
No, it just checks if something was parsed. By the time you reach that function
you've lost track of the length of the string. And there's still the issue of
PR_strtod supporting a wider syntax then you probably want (such as whitespace
and scientific notation)
(In reply to comment #40)
> No, it just checks if something was parsed. By the time you reach that function
> you've lost track of the length of the string. And there's still the issue of
> PR_strtod supporting a wider syntax then you probably want (such as whitespace
> and scientific notation)

Scientific is fine (schema spec allows it, and the duration parsing code would
invalidate it).  I could check the first character for " \t\r\n" to cover the
leading whitespace issue.

Also, wouldn't *pEnd point at the character the parsing broke off at, and
checking if it isn't '\0' for invalid doubles?  That is how I do it for strtol
and works fine.
I'd suggest that you check the code for PR_strtod. Just checking for '\0' won't
catch embedded nulls in the nsAString.
(In reply to comment #42)
> I'd suggest that you check the code for PR_strtod. Just checking for '\0' won't
> catch embedded nulls in the nsAString.

Checked in with the requested changes.  I'll figure out for another patch how to
make it really validate a double.

I do see existing code like nsIVariant return a validatione rror if (pEnd !=
aString), so I assumed they were right.  SVG does  (*end != '\0') as well...
(Assignee)

Comment 44

13 years ago
Created attachment 190772 [details] [diff] [review]
fixed fractional seconds

fixed my handling of fractional seconds to use Doron's change plus parsed
fractional seconds for myself on dateTime since PRTime doesn't contain any.
Attachment #189217 - Attachment is obsolete: true
Attachment #190772 - Flags: review?(bugmail)
Comment on attachment 190772 [details] [diff] [review]
fixed fractional seconds

>+    // Time is usually terminated with Z or followed by a time zone
>+    // (i.e. -05:00).  Time can also be terminated by the end of the string, so
>+    // test for that as well.  All of this specified at:
>+    // http://www.w3.org/TR/xmlschema-2/#dateTime

Would be great if someone else could sign off on that this is the right way to
parse out the fractions. My schema foo isn't quite strong enough.

>+  if (fractionResult.IsEmpty()) {
>+    // couldn't successfully parse the fractional seconds, so we'll just return
>+    // without them.
>+    NS_WARN_IF_FALSE(!fractionResult.IsEmpty(), 
>+      "Couldn't parse the fractional seconds from the given dateTime");

This warning seems pretty useless since you just check that exact same thing
before entering the if-statment.

>+  // convert the result string that we have to a double and add it to the total
>+  const char *cStringValue = NS_ConvertUTF16toUTF8(fractionResult).get();
>+  char *pEnd;
>+  double convertedFraction = PR_strtod(cStringValue, &pEnd);
>+  totalSeconds += convertedFraction;

I'm a bit worried about the lifetime of the result from the
NS_ConvertUTF16toUTF8. And both pEnd and convertedFraction seems pretty
useless. Just do

totalSeconds += PR_strtod(NS_ConvertUTF16toUTF8(fractionResult).get(), nsnull);

Yay! Three nits on one line :)

Actually, you should just make fractionResult an nsCAutoString and do the
conversion while you do the copy.

With that r=me
Attachment #190772 - Flags: review?(bugmail) → review+
(Assignee)

Comment 46

13 years ago
Comment on attachment 190772 [details] [diff] [review]
fixed fractional seconds

Doron, could you check my fraction second parsing to make sure it is right,
please?
Attachment #190772 - Flags: review?(doronr)
Btw, use AppendUTF16toUTF8
(In reply to comment #46)
> (From update of attachment 190772 [details] [diff] [review] [edit])
> Doron, could you check my fraction second parsing to make sure it is right,
> please?
> 

+  const nsAString& fraction = Substring(aValue, findFractionalSeconds+1, 
+                                        aValue.Length());

hmm, do you need to use substring, or could you move the iterator to the right
position in the main string?  I forget if that is possible.
(Assignee)

Comment 49

13 years ago
(In reply to comment #48)
> (In reply to comment #46)
> > (From update of attachment 190772 [details] [diff] [review] [edit] [edit])
> > Doron, could you check my fraction second parsing to make sure it is right,
> > please?
> > 
> 
> +  const nsAString& fraction = Substring(aValue, findFractionalSeconds+1, 
> +                                        aValue.Length());
> 
> hmm, do you need to use substring, or could you move the iterator to the right
> position in the main string?  I forget if that is possible.


according to the string guide, "It is very simple to refer to a substring of an
existing string without actually allocating new space and copying the characters
into that substring. Substring() is the preferred method to create a reference
to such a string."  So I don't think I'd gain much by not using substring.
(Assignee)

Comment 50

13 years ago
Created attachment 191393 [details] [diff] [review]
fixed comments

fixed sicking's comments.  Can you please sr, tor?
Attachment #190772 - Attachment is obsolete: true
Attachment #191393 - Flags: superreview?(tor)
Doh, I missed that |totalSeconds| seems like a useless variable too. No need for
a new patch though.
Attachment #189764 - Flags: superreview?(peterv)

Updated

13 years ago
Attachment #191393 - Flags: superreview?(tor) → superreview+

Comment 52

13 years ago
Comment on attachment 191393 [details] [diff] [review]
fixed comments

a=mkaply
Attachment #191393 - Flags: approval1.8b4+
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Attachment #190772 - Flags: review?(doronr)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.