Closed Bug 1434007 Opened 6 years ago Closed 6 years ago

Implement String.prototype.trimStart and String.prototype.trimEnd

Categories

(Core :: JavaScript Engine, enhancement, P3)

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: alex.fdm, Assigned: anba)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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.
Blocks: test262
Attached patch bug1434007.patchSplinter Review
- 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.
Attachment #8947818 - Flags: review?(evilpies)
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.
Attachment #8947818 - Flags: review?(evilpies) → review+
(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.
Priority: -- → P3
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4872ba5521
Implement String.prototype.trim{Start,End} stage 3 proposal. r=evilpie
Keywords: checkin-needed
(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?
Attachment #8954298 - Flags: review?(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 :/
Attachment #8954298 - Flags: review?(till) → review+
Updated to apply cleanly on inbound, carrying r+.
Attachment #8954298 - Attachment is obsolete: true
Attachment #8961583 - Flags: review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6771d444d64f
Enable String.prototype.trim{Start,End} by default. r=till
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6771d444d64f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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!
Thanks, fixed :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: