220348 - Use native .trim*() throughout mozilla-central

Status: NEW

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Emmanuel Bourg]

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.

Comment 1

[cls]

-> B-G

Comment 2

[John Resig]

It seems like this could be helped by bug 305064.

Comment 3

[Egon Knapen]

Like I said in my bug report, use it a lot everywhere.... Still missing it...

Comment 4

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v1

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.

Comment 5

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v2

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.

Comment 6

[Serge Gautherie (:sgautherie)]

Let's morph this bug to Core.

I would suggest to split the current patch into separate areas/bugs...

Comment 7

[Steffen Wilberg]

Morphing summary, and removing dependencies for Thunderbird/Seamonkey/Calendar/Editor parts; those only depend on bug 305064.

Comment 8

[Serge Gautherie (:sgautherie)]

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

Comment 9

[Steffen Wilberg]

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.

Comment 10

[Steffen Wilberg]

Er, mozilla/mailnews was bug 471845.

Comment 11

[Dão Gottwald [:dao]]

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?

Comment 12

[Nochum Sossonko [:Natch]]

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

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 337385 [details] [diff] [review]
Patch v2

the consoleBindings.xml change looks wrong

Comment 14

[Dão Gottwald [:dao]]

Mike, are you going to finish this?

Comment 15

[Mike Kaplinskiy [:mikekap] (at school, no time)]

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.

Comment 16

[Dão Gottwald [:dao]]

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

Comment 17

[Gregory Pappas [:gregp]]

Attachment: Bug 220348 - Use native .trim*() throughout mozilla-central r?gijs!,robwu!

Comment 18

[Gregory Pappas [:gregp]]

Not actively working on this