Closed Bug 471845 Opened 11 years ago Closed 11 years ago

Use .trim*() in SeaMonkey

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a3

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

Attachments

(3 files)

No description provided.
Blocks: TB2SM
Blocks: 220348
Depends on: 305064
Target Milestone: --- → seamonkey2.0a3
Summary: Port |Bug 454125 - Use .trim() in Thunderbird| to SeaMonkey → Use .trim*() in SeaMonkey
This is untested, but should be trivial enough.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #355106 - Flags: superreview?(neil)
Attachment #355106 - Flags: review?(neil)
This is untested, but should be trivial enough.
Attachment #355107 - Flags: review?(neil)
This is untested, but should be trivial enough.
Attachment #355108 - Flags: superreview?(neil)
Attachment #355108 - Flags: review?(neil)
Comment on attachment 355107 [details] [diff] [review]
(Bv1) /suite/ part
[Checkin: See comment 8]

> // Copied from the Links Panel v2.3, http://segment7.net/mozilla/links/links.html
> // strip leading and trailing whitespace, and replace multiple consecutive whitespace characters with a single space
> function stripWS(text)
> {
>-  var middleRE = /\s+/g;
>-  var endRE = /(^\s+)|(\s+$)/g;
>-
>-  text = text.replace(middleRE, " ");
>-  return text.replace(endRE, "");
>+  return text.trim().replace(/\s+/g, " ");
> }
Nit: the first comment line no longer makes sense.

>+    var tmp = node.nodeValue.replace(/(\n|\r|\t)+/g, " ").trim();
Nit: might as well put the trim first

>-  var url = site.value.replace(/^\s*([-\w]*:\/+)?/, "http://");
>+  var url = site.value.trimLeft().replace(/^([-\w]*:\/+)?/, "http://");

>-  var site = textfield.value.replace(/^\s*([-\w]*:\/+)?/, "");
>+  var site = textfield.value.trimLeft().replace(/^[-\w]*:\/+/, "");
I don't think these changes improve the code. Please remove them.
Attachment #355107 - Flags: review?(neil) → review+
Comment on attachment 355106 [details] [diff] [review]
(Av1) Port TB bug 454125
[Checkin: See comment 7]

>-  var type = aData.type.toLowerCase().replace(/^\s+|\s*(?:;.*)?$/g, "");
>+  var type = aData.type.toLowerCase()
>+                       .trimLeft().replace(/(?:;.*)?$/g, "").trimRight();
I don't think this change is right (or worthwhile). Please remove it.
Attachment #355106 - Flags: superreview?(neil)
Attachment #355106 - Flags: superreview+
Attachment #355106 - Flags: review?(neil)
Attachment #355106 - Flags: review+
Comment on attachment 355108 [details] [diff] [review]
(Cv1) /mailnews/ part
[Checkin: Comment 9]

> function trim(string)
> {
>-  return string.replace(/(^\s+)|(\s+$)/g, "");
>+  return string.trim();
> }
We should really fix the callers here.
Attachment #355108 - Flags: superreview?(neil)
Attachment #355108 - Flags: superreview+
Attachment #355108 - Flags: review?(neil)
Attachment #355108 - Flags: review+
Comment on attachment 355106 [details] [diff] [review]
(Av1) Port TB bug 454125
[Checkin: See comment 7]

http://hg.mozilla.org/comm-central/rev/4d00c64e4a28
with comment 5 suggestion(s).
Attachment #355106 - Attachment description: (Av1) Port TB bug 454125 → (Av1) Port TB bug 454125 [Checkin: See comment 7]
Comment on attachment 355107 [details] [diff] [review]
(Bv1) /suite/ part
[Checkin: See comment 8]

http://hg.mozilla.org/comm-central/rev/4f78e0be4a78
with comment 4 suggestion(s).

(In reply to comment #4)
> >+    var tmp = node.nodeValue.replace(/(\n|\r|\t)+/g, " ").trim();
> Nit: might as well put the trim first

I did not:
It would change the behavior from the current code and I don't want to do that in this patch.
Besides, I think it's needed like this, in case |replace()| "adds" leading/trailing spaces.

Yet, I can do it in a followup patch if you do want this change.
Attachment #355107 - Attachment description: (Bv1) /suite/ part → (Bv1) /suite/ part [Checkin: See comment 8]
Attachment #355108 - Attachment description: (Cv1) /mailnews/ part → (Cv1) /mailnews/ part [Checkin: Comment 9]
Comment on attachment 355108 [details] [diff] [review]
(Cv1) /mailnews/ part
[Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/bb65d1541172

(In reply to comment #6)
> > function trim(string)
> We should really fix the callers here.

I noticed it too, but I preferred to leave this (separate) for bug 220348 (followup)...
No longer blocks: TB2SM
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
No longer blocks: 220348
> I guess you don't care about wallet anymore?

Since we (and thunderbird) are moving to satchel real soon now, there won't be any consumers left, so nobody cares about wallet any more.
You need to log in before you can comment on or make changes to this bug.