Closed
Bug 281987
Opened 19 years ago
Closed 19 years ago
Finish out XPath extension functions
Categories
(Core Graveyard :: XForms, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aaronr, Assigned: aaronr)
References
Details
Attachments
(6 files, 8 obsolete files)
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
|
tor
:
superreview+
mkaply
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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.
Comment 1•19 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
I'm implementing index() under 282777
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().
ready for review now that pre-reqs are in
Attachment #182937 -
Attachment is obsolete: true
Attachment #187712 -
Flags: review?(doronr)
Comment 9•19 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•19 years ago
|
||
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•19 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.
Comment 14•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
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•19 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•19 years ago
|
||
Lousy calendar system.
Comment 22•19 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•19 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.
Comment 25•19 years ago
|
||
Assignee | ||
Comment 26•19 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 27•19 years ago
|
||
Comment on attachment 189764 [details] [diff] [review] make schema duration return doubles asking for sr :)
Attachment #189764 -
Flags: superreview?(peterv)
Assignee | ||
Comment 28•19 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-
Comment 30•19 years ago
|
||
(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)
Comment 33•19 years ago
|
||
Attachment #189764 -
Attachment is obsolete: true
Attachment #190468 -
Flags: review?(bugmail)
Comment 34•19 years ago
|
||
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-
Comment 36•19 years ago
|
||
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.
Comment 39•19 years ago
|
||
(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)
Comment 41•19 years ago
|
||
(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.
Comment 43•19 years ago
|
||
(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•19 years ago
|
||
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•19 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
Comment 48•19 years ago
|
||
(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•19 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•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #189764 -
Flags: superreview?(peterv)
Attachment #191393 -
Flags: superreview?(tor) → superreview+
Comment 52•19 years ago
|
||
Comment on attachment 191393 [details] [diff] [review] fixed comments a=mkaply
Attachment #191393 -
Flags: approval1.8b4+
Comment 53•19 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #190772 -
Flags: review?(doronr)
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•