Implemented String.prototype.endsWith as per ES6 draft 07-15-13, section 15.5.3.23

Implemented String.prototype.endsWith as per ES6 draft 07-15-13, section 15.5.3.23

TEST=mjsunit/string-endswith
BUG=v8:2798

[Michael Starzinger, 2013-07-29 13:16:26]

First round of comments.

Also there is the general question of whether these new functions on
String.prototype should be put behind the --harmony flag or not. Personally I
think they are pretty simple and I don't have a beef with exposing them right
away, but we need to be cautious. I'll loop in more people in this discussion.

https://codereview.chromium.org/20928002/diff/1/src/string.js
File src/string.js (right):

https://codereview.chromium.org/20928002/diff/1/src/string.js#newcode894
src/string.js:894: function StringEndsWith(searchString /* position */) { //
length == 1
nit: Two white-spaces before comment at end of line.

https://codereview.chromium.org/20928002/diff/1/src/string.js#newcode907
src/string.js:907: if (%_ArgumentsLength() > 1) {
There is a missing check as to whether "position" is explicitly passed an
"undefined" value, in which case "pos" should remain s.length here.

Can we add a test-case for that?

https://codereview.chromium.org/20928002/diff/1/src/string.js#newcode909
src/string.js:909: if (!NUMBER_IS_NAN(arg)) {
In case ToNumber(position) yields NaN, then "pos" should be set to 0 IIUC.
Better use the internal ToInteger() helper here, that should do the right thing.

Can we add a test-case for that?

https://codereview.chromium.org/20928002/diff/1/src/string.js#newcode914
src/string.js:914: var end = $Math.min($Math.max(pos, 0), s_len);
This calls potentially monkey-patched versions of Math.min() and Math.max(),
better directly call the MathMin() and MathMax() implementations directly.

[Michael Starzinger, 2013-07-29 13:55:23]

Yep, as suspected, we need to put this new functionality behind the --harmony
flag. The easiest way to do this that I can think of is to move the methods into
a separate JS file (e.g. harmony.js) that is evaluated after context
deserialization. If you take a look at Genesis::InstallExperimentalNatives(),
you'll see how that's done for the other features. Inside the harmony.js file
there should then be some SetUpHarmonyString() method that extends the existing
string prototype with the new ES6 functions. AFAICT the InstallFunctions()
helper should be flexible enough to be called twice on the same object. This
applies to all three of your CLs.

[ioseb, 2013-07-29 17:22:12]

On 2013/07/29 13:55:23, Michael Starzinger wrote:
> Yep, as suspected, we need to put this new functionality behind the --harmony
> flag. The easiest way to do this that I can think of is to move the methods
into
> a separate JS file (e.g. harmony.js) that is evaluated after context
> deserialization. If you take a look at Genesis::InstallExperimentalNatives(),
> you'll see how that's done for the other features. Inside the harmony.js file
> there should then be some SetUpHarmonyString() method that extends the
existing
> string prototype with the new ES6 functions. AFAICT the InstallFunctions()
> helper should be flexible enough to be called twice on the same object. This
> applies to all three of your CLs.

@Michael

Thanks for guidance. I've figured out everything regarding --harmony flag. Just
one question:

Should i combine all of three CLs in one patch and resubmit it?

[Michael Starzinger, 2013-07-29 17:42:18]

Yes, I am fine with combining all three CLs into one.

[ioseb, 2013-07-29 23:26:21]

Michael, I combined all of three CLs in separate file and implemented as you
suggested: https://codereview.chromium.org/21014007

Additionally added String.prototype.contains method which is also in the spec.

P.S.
I think we can close these old CLs?

On 2013/07/29 17:42:18, Michael Starzinger wrote:
> Yes, I am fine with combining all three CLs into one.

[Michael Starzinger, 2013-07-30 09:02:51]

Superseded by https://codereview.chromium.org/21014007. Closed.