Closed
Bug 471860
Opened 16 years ago
Closed 15 years ago
Use .trim*() in Calendar
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b1
People
(Reporter: sgautherie, Assigned: sgautherie)
References
Details
Attachments
(1 file)
2.51 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•16 years ago
|
||
This is untested,
and I have a doubt about the calWcapSession.js change (which may need an anonymous wrapper function)...
Comment 2•16 years ago
|
||
Comment on attachment 355100 [details] [diff] [review]
(Av1) Remove trimString() ++
># HG changeset patch
># User Serge Gautherie <sgautherie.bz@free.fr>
>Bug 471860 - Use .trim*() in Calendar
>(Av1) Remove trimString() ++
>
>diff --git a/calendar/base/src/calProviderUtils.js b/calendar/base/src/calProviderUtils.js
>--- a/calendar/base/src/calProviderUtils.js
>+++ b/calendar/base/src/calProviderUtils.js
>@@ -114,19 +114,18 @@ function convertByteArray(aResult, aResu
> throw e;
> }
> }
> return null;
> }
>
> function safeNewXML(aStr) {
> // Strip <?xml and surrounding whitespaces
>- return new XML(aStr.replace(/(^\s*(<\?xml[^>]*>)?\s*|\s+$)/g, ""));
>+ return new XML(aStr.trim().replace(/^<\?xml[^>]*>/g, "").trimLeft());
Why trim twice?
>+ ret = ret.concat(i.split(/[;,]/).map(trim));
I believe we need something like map(String.prototype.trim), I'll test this later on.
I'll r+ after I've tested this afternoon.
Assignee | ||
Comment 3•16 years ago
|
||
(In reply to comment #2)
> >- return new XML(aStr.replace(/(^\s*(<\?xml[^>]*>)?\s*|\s+$)/g, ""));
> >+ return new XML(aStr.trim().replace(/^<\?xml[^>]*>/g, "").trimLeft());
> Why trim twice?
Iiuc,
trim() replaces ^\s* and \s+$,
then trimLeft() replaces \s*, after the (optional) xml element removal.
I though it could be written aStr.replace(/<\?xml[^>]*>/g, "").trim() instead,
but removing the leading ^ might change some edge cases behavior...
I leave it up to you to know/decide.
Comment 4•16 years ago
|
||
(In reply to comment #3)
> I though it could be written aStr.replace(/<\?xml[^>]*>/g, "").trim() instead,
> but removing the leading ^ might change some edge cases behavior...
I decided to just trimRight() at the end and use the regex for the beginning, since we need to execute the regex anyway.
I also changed the map() call to arr.map(String.trim), which seems to work in xpcshell.
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6f833beb10d0>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Updated•16 years ago
|
Attachment #355100 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 5•16 years ago
|
||
(In reply to comment #4)
trimRight() replaces \s+$ :-)
The 2 other changes you made/left to the regex might change some edge cases behavior, afaict,
but if you know the new regex is better/enough than the previous one, that's fine with me...
Comment 6•16 years ago
|
||
(In reply to comment #5)
> trimRight() replaces \s+$ :-)
Isn't that correct? I'm replacing ^\s* in the regex and using trimRight() to get rid of the end.
>
> The 2 other changes you made/left to the regex might change some edge cases
> behavior, afaict,
> but if you know the new regex is better/enough than the previous one, that's
> fine with me...
The old regex didn't escape the /, which causes by syntax checker to bork and probably isn't right. We never noticed since we just catch all exceptions there.
Assignee | ||
Comment 7•16 years ago
|
||
(In reply to comment #6)
> Isn't that correct? I'm replacing ^\s* in the regex and using trimRight() to
> get rid of the end.
^\s* was not replaced, was it ? (and I'm not saying it should be.)
trimRight() replaces \s+$ :
that's obvious.
trimRight() replaces ()?\s* too :
yes, but not if there were a line like "<xml> <other>" for example.
I was saying that if you know that the only non-"space" characters can only be the xml element then the new regex is fine/simpler, otherwise it looks like a regression which may leave "spaces" at the beginning of the result.
> The old regex didn't escape the /, which causes by syntax checker to bork and
> probably isn't right. We never noticed since we just catch all exceptions
> there.
I see no / to escape !?
Comment 8•15 years ago
|
||
(In reply to comment #7)
> yes, but not if there were a line like "<xml> <other>" for example.
>
> I was saying that if you know that the only non-"space" characters can only be
> the xml element then the new regex is fine/simpler, otherwise it looks like a
> regression which may leave "spaces" at the beginning of the result.
Yeah, this patch is definitely incorrect. It should be trim.replace.trimLeft or trimLeft.replace.trim.
Comment 9•15 years ago
|
||
Then we should reopen it to reconsider.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•15 years ago
|
||
These all should work:
return new XML(aStr.trim().replace(/^<\?xml[^>]*>/, "").trimLeft());
or
return new XML(aStr.trim().replace(/^<\?xml[^>]*>\s*/, ""));
or
return new XML(aStr.trimLeft().replace(/^<\?xml[^>]*>/, "").trim());
Note that the global flag is not needed.
Assignee | ||
Comment 11•15 years ago
|
||
(In reply to comment #10)
> return new XML(aStr.trim().replace(/^<\?xml[^>]*>/, "").trimLeft());
Yes, see my patch ;-)
> Note that the global flag is not needed.
Probably yes, though I leave it to you to know about that (now).
Philipp, can you just fix theses...
Comment 12•15 years ago
|
||
Ah, now I see why my suggesstion:
return new XML(aStr.replace(/^\s*<\?xml[^>]*>/g, "").trimRight());
is wrong. Sorry for my ignorance. I'll go with:
return new XML(aStr.trim().replace(/^<\?xml[^>]*>\s*/, ""));
since its only one trim call and we have to do the regex anyway.
Pushed as changeset 8d779ed71cbb.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #12)
> return new XML(aStr.trim().replace(/^<\?xml[^>]*>\s*/, ""));
Fwiw, the point of this bug was to use the native functions,
so this is the one, out of the three, which I wouldn't have used.
But, anyway.
Comment 14•13 years ago
|
||
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•