1434007 - Implement String.prototype.trimStart and String.prototype.trimEnd

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Alexandre Folle de Menezes]

The string trimming proposal is already at STAGE3, and is due to be approved soon:
https://github.com/sebmarkbage/ecmascript-string-left-right-trim

Firefox already implements String.prototype.trimLeft and String.prototype.trimRight, but String.prototype.trimStart and String.prototype.trimEnd are missing.

Comment 1

[André Bargull [:anba]]

Attachment: bug1434007.patch

- Renames internal uses of trimLeft/Right with trimStart/End.
- Doesn't add String.trimStart/End because we want to remove String generics and adding new ones is kind of counter-productive. :-)
- The new trimLeft/End functions are currently restricted to nightly-only as long as they're only at stage 3. While I don't expect any compatibility issues, it's probably safer to avoid letting them ride the trains for a release or two.


Tests will be added through bug 1434953.

Comment 2

[Tom S. (please needinfo tschuster)]

Comment on attachment 8947818 [details] [diff] [review]
bug1434007.patch

Review of attachment 8947818 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +3325,5 @@
>      JS_FN("trim",              str_trim,              0,0),
> +#ifdef NIGHTLY_BUILD
> +    JS_FN("trimStart",         str_trimStart,         0,0),
> +    JS_FN("trimEnd",           str_trimEnd,           0,0),
> +#else

I thought this might break JS Xrays, but String doesn't seem to use ClassSpec at all.

Comment 3

[Till Schneidereit [:till]]

(In reply to André Bargull [:anba] from comment #1)
> - The new trimLeft/End functions are currently restricted to nightly-only as
> long as they're only at stage 3. While I don't expect any compatibility
> issues, it's probably safer to avoid letting them ride the trains for a
> release or two.

FWIW I think it'd be fine to just ship these: they're simple enough that there's no churn to be expected, and shipping in release browsers is a requirement for advancing to stage 4.

Comment 4

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1bdc2b8cff1c8d600fb5bfc03fae9f70a45b5c

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4872ba5521
Implement String.prototype.trim{Start,End} stage 3 proposal. r=evilpie

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7e4872ba5521

Comment 7

[André Bargull [:anba]]

Attachment: bug1434007-enable-in-release.patch

(In reply to Till Schneidereit [:till] from comment #3)
> FWIW I think it'd be fine to just ship these: they're simple enough that
> there's no churn to be expected, and shipping in release browsers is a
> requirement for advancing to stage 4.

We didn't see any bug reports mentioning trimStart/trimEnd and V8 also seems to ship trimStart/trimEnd in releases <https://bugs.chromium.org/p/v8/issues/detail?id=6530#c4>. The questions is, do we want to enable it by default for Firefox 60, which means also the next ESR, or do we want to wait for Firefox 61?

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8954298 [details] [diff] [review]
bug1434007-enable-in-release.patch

Review of attachment 8954298 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the late reply - I was out sick last week. At least that got us around the question whether to ship this in 60 or not, given that it's now too late for that :/

Comment 9

[André Bargull [:anba]]

Attachment: bug1434007-enable-in-release.patch

Updated to apply cleanly on inbound, carrying r+.

Comment 10

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13378bb17341a1505c614448df0221716fb67021

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6771d444d64f
Enable String.prototype.trim{Start,End} by default. r=till

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6771d444d64f

Comment 13

[Florian Scholz (Open Web Docs)]

Announced here:
https://developer.mozilla.org/en-US/Firefox/Releases/61#JavaScript

Updated:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trimStart
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trimEnd

Interactive example and compat data are still pending review, but will get done here:
https://github.com/mdn/interactive-examples/pull/922
https://github.com/mdn/browser-compat-data/pull/2013

Let me know if this looks OK to you.

Comment 14

[André Bargull [:anba]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #13)
> Let me know if this looks OK to you.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trimEnd has a typo in its first line: "trimend" instead of "trimEnd". Otherwise lgtm!

Comment 15

[Florian Scholz (Open Web Docs)]

Thanks, fixed :)