Closed Bug 471860 Opened 11 years ago Closed 10 years ago

Use .trim*() in Calendar

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

Attachments

(1 file)

No description provided.
This is untested,
and I have a doubt about the calWcapSession.js change (which may need an anonymous wrapper function)...
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #355100 - Flags: review?(philipp)
No longer blocks: 220348
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.
(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.
(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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Attachment #355100 - Flags: review?(philipp) → review+
(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...
(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.
(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 !?
(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.
Then we should reopen it to reconsider.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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...
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: 11 years ago10 years ago
Resolution: --- → FIXED
(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.
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.