Open Bug 220348 Opened 16 years ago Updated 9 years ago

Use native .trim*() throughout mozilla-central

Categories

(Core :: General, enhancement)

enhancement
Not set

Tracking

()

People

(Reporter: ebourg, Unassigned)

References

Details

(Keywords: helpwanted, perf)

Attachments

(1 file, 1 obsolete file)

While browing the code I noticed a trim() function is defined in many different
places :

mozilla/editor/ui/composer/content/editorUtilities.js:
TrimStringLeft(string)
TrimStringRight(string)
TrimString(string)

mozilla/extensions/cview/resources/content/cview-utils.js:
stringTrim(s)

mozilla/extensions/irc/js/lib/utils.js:
stringTrim(s)

mozilla/extensions/venkman/resources/content/venkman-utils.js
stringTrim(s)

mozilla/extensions/wallet/signonviewer/SignonViewer.js
TrimString(string)

mozilla/js/test/ecma_3/Date/15.9.5.4.js
trimL(s)
trimR(s)

mozilla/js/test/ecma_3/Date/15.9.5.7.js
trimL(s)
trimR(s)

mozilla/mailnews/base/search/resources/content/CustomHeaders.js
TrimString(string)

mozilla/toolkit/components/passwordmgr/resources/content/passwordManager.js
TrimString(string)

mozilla/tools/wizards/templates/xul-app/resources/content/app-utils.js
stringTrim(s)


I'd suggest to put a unique trim() method in a common package. This may be the
opportunity to start a javascript utility package that would gather common
methods used in various extensions.
-> B-G
Assignee: mozbugs-build → general
Component: Build Config → Browser-General
QA Contact: general
Product: Browser → Seamonkey
It seems like this could be helped by bug 305064.
Depends on: 305064
Like I said in my bug report, use it a lot everywhere.... Still missing it...
Attached patch Patch v1 (obsolete) — Splinter Review
For after bug 305064 lands.

This replaces pretty much all trim-looking operations (*trim*, *.replace(/^\s+|\s+$/g, '')).

This would be relatively low risk except some of the trim functions weren't completely simple; e.g. TrimString@editor/ui/composer/content/editorUtilities.js where TrimString(null/undefined) == ''. So some of the refactoring has conditionals since I can't tell for sure what will be null/undefined.

Comments are welcome. When 305064 ever lands, I'll mark this for review.
Attached patch Patch v2Splinter Review
Ok, this is more complete. (thanks to philor) I also removed a bad change in js tests and a change in mochikit.

New bug for composer/editor: bug 454129.

I'm guessing this may need multiple reviews, but a lot of it is in the browser.
Assignee: general → mike.kaplinskiy
Attachment #332666 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #337385 - Flags: review?(gavin.sharp)
Depends on: 454090
Depends on: 471860
Depends on: 471845
Let's morph this bug to Core.

I would suggest to split the current patch into separate areas/bugs...
Product: SeaMonkey → Core
QA Contact: general → general
Morphing summary, and removing dependencies for Thunderbird/Seamonkey/Calendar/Editor parts; those only depend on bug 305064.
No longer depends on: 454125, 454129, 471845, 471860
Summary: Multiple trim() functions → Use native .trim*() throughout mozilla-central
(In reply to comment #7)
> Morphing summary, and removing dependencies for
> Thunderbird/Seamonkey/Calendar/Editor parts; those only depend on bug 305064.

Humh, then can you please file a new (dependent) bug to unify the multiple functions (in these apps), per comment 0.
OK, filed bug 472393 for Chatzilla and bug 472395 for Venkman.

mozilla/toolkit/components/passwordmgr/resources/content/passwordManager.js is part of the patch in this bug.
mozilla/js/test/ecma_3 should be covered in this bug.

mozilla/editor/ui was/is bug 454129.
mozilla/extensions/wallet is bug 471845 (Seamonkey) and/or bug 454125 (Thunderbird), if they still care.
mozilla/mailnews was bug 454125.
mozilla/extensions/cview and mozilla/tools/wizards doesn't exist anymore.
Er, mozilla/mailnews was bug 471845.
Comment on attachment 337385 [details] [diff] [review]
Patch v2

>diff --git a/toolkit/components/console/content/consoleBindings.xml b/toolkit/components/console/content/consoleBindings.xml
>--- a/toolkit/components/console/content/consoleBindings.xml
>+++ b/toolkit/components/console/content/consoleBindings.xml
>@@ -179,17 +179,17 @@
>           row.setAttribute("category", aObject.category);
>           if (aObject.lineNumber || aObject.sourceName) {
>             row.setAttribute("href", aObject.sourceName);
>             row.setAttribute("line", aObject.lineNumber);
>           } else {
>             row.setAttribute("hideSource", "true");
>           }
>           if (aObject.sourceLine) {
>-            row.setAttribute("code", aObject.sourceLine.replace(/\s/g, " "));
>+            row.setAttribute("code", aObject.sourceLine.replace(/\s+/g, " "));

What's that change about?
I haven't actually tested the changes, but there should be _some_ perf gain involved in this change, RegExp are relatively slow (IIRC they aren't traced yet) and native methods are generally faster than by-hand stuff...
Flags: wanted1.9.2?
Keywords: perf
Attachment #337385 - Flags: review?(gavin.sharp) → review-
Comment on attachment 337385 [details] [diff] [review]
Patch v2

the consoleBindings.xml change looks wrong
Mike, are you going to finish this?
I don't really have time to work on this right now, so if anyone wants to pick this up, i'll let go of the bug.

As for the console bindings change - it's really the same thing. I think that the \s+ might be faster, but if you don't like it, feel free to leave it out.
Assignee: mike.kaplinskiy → nobody
Status: ASSIGNED → NEW
(In reply to comment #15)
> As for the console bindings change - it's really the same thing.

No, it's not. One transforms "\t\n " to three spaces, the other to one space.
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.