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

Status: RESOLVED FIXED

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

Description

[Steffen Wilberg]

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

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: proposed patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 403862 [details] [diff] [review]
proposed patch

r=me

Comment 3

[:Gijs (he/him)]

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!

Comment 4

[James Ross]

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) };

Comment 5

[:Gijs (he/him)]

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

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

This has the added benefit of not having to change the Add Watch Expression context menu support that was just checked in. :-)

Comment 7

[:Gijs (he/him)]

Comment on attachment 404260 [details] [diff] [review]
patch v2

Cheers! Will check this in in a bit.

Comment 8

[:Gijs (he/him)]

http://hg.mozilla.org/venkman/rev/7d45be8a1900, thanks!