471860 - Use .trim*() in Calendar

Status: RESOLVED FIXED

(Calendar :: General, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Remove trimString() ++

This is untested,
and I have a doubt about the calWcapSession.js change (which may need an anonymous wrapper function)...

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 355100 [details] [diff] [review]
(Av1) Remove trimString() ++

># HG changeset patch
># User Serge Gautherie <[e-mail]>
>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.

Comment 3

[Serge Gautherie (:sgautherie)]

(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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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

Comment 5

[Serge Gautherie (:sgautherie)]

(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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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.

Comment 7

[Serge Gautherie (:sgautherie)]

(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

[Michael Ryan]

(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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Then we should reopen it to reconsider.

Comment 10

[Michael Ryan]

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.

Comment 11

[Serge Gautherie (:sgautherie)]

(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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 13

[Serge Gautherie (:sgautherie)]

(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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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".