Closed Bug 472395 Opened 11 years ago Closed 11 years ago

replace custom stringTrim() by native .trim() in Venkman

Categories

(Other Applications Graveyard :: Venkman JS Debugger, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: steffen.wilberg, Assigned: InvisibleSmiley)

References

Details

Attachments

(1 file, 1 obsolete file)

Split from bug 220348 comment 0:
The custom stringTrim() function should be replaced by the new native .trim()
function introduced by bug 305064.

http://mxr.mozilla.org/comm-central/search?string=stringTrim&find=\.js&findi=\.js&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Attached patch proposed patch (obsolete) — Splinter Review
Attachment #403862 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 403862 [details] [diff] [review]
proposed patch

r=me
Attachment #403862 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee: rginda → jh
Status: NEW → ASSIGNED
Severity: normal → trivial
Keywords: checkin-needed
Merm, actually, apparently it's not this simple... The latest release versions of Sunbird and Flock do not support JS 1.8.1, which is when .trim() was introduced (apparently). I'm not sure if there will be versions that do, and when those will be here, or what the 'trunk' version number for them is (whereas it is obvious that SM 2 and TB 3 will support it). Could you pretty please do a patch which keeps stringTrim? You could add the following to utils.js to use trim() where possible (after declaring stringTrim)

if ("trim" in String.prototype)
    stringTrim = function _strtrim(str) { return str.trim() };

Sorry for the false positive!
You could do the reverse and still make the rest of the code cleaner:

if (!("trim" in String.prototype))
    String.prototype.trim = function() { return stringTrim(this) };
(In reply to comment #4)
> You could do the reverse and still make the rest of the code cleaner:
> 
> if (!("trim" in String.prototype))
>     String.prototype.trim = function() { return stringTrim(this) };

I considered that, but don't *really* like messing with basic prototypes. I guess since this is only in our own window, we can get away with it, and indeed that would be slightly more performant. :-)
Keywords: checkin-needed
Attached patch patch v2Splinter Review
This has the added benefit of not having to change the Add Watch Expression context menu support that was just checked in. :-)
Attachment #403862 - Attachment is obsolete: true
Attachment #404260 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 404260 [details] [diff] [review]
patch v2

Cheers! Will check this in in a bit.
Attachment #404260 - Flags: review?(gijskruitbosch+bugs) → review+
http://hg.mozilla.org/venkman/rev/7d45be8a1900, thanks!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.