Open
Bug 220348
Opened 21 years ago
Updated 10 months ago
Use native .trim*() throughout mozilla-central
Categories
(Core :: General, enhancement)
Core
General
Tracking
()
ASSIGNED
People
(Reporter: ebourg, Assigned: gregp)
References
Details
(Keywords: helpwanted, perf)
Attachments
(2 files, 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.
Updated•21 years ago
|
Keywords: helpwanted
-> B-G
Assignee: mozbugs-build → general
Component: Build Config → Browser-General
QA Contact: general
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 3•16 years ago
|
||
Like I said in my bug report, use it a lot everywhere.... Still missing it...
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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)
Comment 6•16 years ago
|
||
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
Comment 7•16 years ago
|
||
Morphing summary, and removing dependencies for Thunderbird/Seamonkey/Calendar/Editor parts; those only depend on bug 305064.
Comment 8•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
Er, mozilla/mailnews was bug 471845.
Comment 11•15 years ago
|
||
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•15 years ago
|
||
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
Updated•15 years ago
|
Attachment #337385 -
Flags: review?(gavin.sharp) → review-
Comment 13•15 years ago
|
||
Comment on attachment 337385 [details] [diff] [review] Patch v2 the consoleBindings.xml change looks wrong
Comment 14•15 years ago
|
||
Mike, are you going to finish this?
Comment 15•15 years ago
|
||
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
Comment 16•15 years ago
|
||
(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.
Updated•13 years ago
|
Flags: wanted1.9.2?
Updated•2 years ago
|
Severity: normal → S3
Assignee | ||
Comment 17•10 months ago
|
||
Updated•10 months ago
|
Assignee: nobody → gp3033
Status: NEW → ASSIGNED
You need to log in
before you can comment on or make changes to this bug.
Description
•