Move ArrayBuffer.prototype.slice implementation to C++

Move ArrayBuffer.prototype.slice implementation to C++

This will be useful for sharing the implementation with
SharedArrayBuffer.prototype.slice.

BUG=v8:5897
Review-Url: https://codereview.chromium.org/2697013009
Cr-Commit-Position: refs/heads/master@{#43503}
Committed: https://chromium.googlesource.com/v8/v8/+/cb8fb46aa3f1bc785f89bc16ad389f7a9b4fe982

[binji, 2017-02-17 00:30:21]

binji@chromium.org changed reviewers:
+ littledan@chromium.org

[binji, 2017-02-17 00:30:32]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 00:30:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 01:20:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 01:20:50]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-02-17 09:16:40]

Description was changed from

==========
Move ArrayBuffer.prototype.slice implementation to C++

This will be useful for sharing the implementation with
SharedArrayBuffer.prototype.slice.

BUG=https://bugs.chromium.org/p/v8/issues/detail?id=5897
==========

to

==========
Move ArrayBuffer.prototype.slice implementation to C++

This will be useful for sharing the implementation with
SharedArrayBuffer.prototype.slice.

BUG=v8:5897
==========

[Dan Ehrenberg, 2017-02-17 09:18:20]

lgtm

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
File src/builtins/builtins-arraybuffer.cc (right):

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
src/builtins/builtins-arraybuffer.cc:92: // ES6 section 24.1.4.3
ArrayBuffer.prototype.slice ( start, end )
Optional nit: As long as you're updating the cross-references, could you follow
the newer convention of referring to section anchors, which are more stable?
Here, the comment might read as

// ES #sec-arraybuffer.prototype.slice
// ArrayBuffer.prototype.slice ( start, end )

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
src/builtins/builtins-arraybuffer.cc:223:
CHECK(NumberToSize(new_array_buffer->byte_length()) >= new_len_size);
Should this last one be a DCHECK? Anyway, this (and the next comment) are not
very important, and it's reasonable to leave this code unchanged from the place
you took it from.

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
src/builtins/builtins-arraybuffer.cc:228: CHECK(from_byte_length - first_size >=
new_len_size);
DCHECKs?

[Dan Ehrenberg, 2017-02-17 09:19:21]

lgtm

By the way, I like the detailed comments you included. I also edited the commit
to point to the bug thread in a way that Rietveldt might understand better.

[binji, 2017-02-17 18:59:19]

On 2017/02/17 09:19:21, Dan Ehrenberg wrote:
> lgtm
> 
> By the way, I like the detailed comments you included.

Thanks, I stole it from somewhere else, I think. :) It's a bit verbose, but
makes it nice and clear.

> I also edited the commit to point to the bug thread in a way that Rietveldt
might understand better.

Oh, didn't know you could do that. Nice!

[binji, 2017-02-17 18:59:28]

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
File src/builtins/builtins-arraybuffer.cc (right):

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
src/builtins/builtins-arraybuffer.cc:92: // ES6 section 24.1.4.3
ArrayBuffer.prototype.slice ( start, end )
On 2017/02/17 09:18:19, Dan Ehrenberg wrote:
> Optional nit: As long as you're updating the cross-references, could you
follow
> the newer convention of referring to section anchors, which are more stable?
> Here, the comment might read as
> 
> // ES #sec-arraybuffer.prototype.slice
> // ArrayBuffer.prototype.slice ( start, end )

Done.

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
src/builtins/builtins-arraybuffer.cc:223:
CHECK(NumberToSize(new_array_buffer->byte_length()) >= new_len_size);
On 2017/02/17 09:18:19, Dan Ehrenberg wrote:
> Should this last one be a DCHECK? Anyway, this (and the next comment) are not
> very important, and it's reasonable to leave this code unchanged from the
place
> you took it from.

Seems reasonable, done.

https://codereview.chromium.org/2697013009/diff/1/src/builtins/builtins-array...
src/builtins/builtins-arraybuffer.cc:228: CHECK(from_byte_length - first_size >=
new_len_size);
On 2017/02/17 09:18:19, Dan Ehrenberg wrote:
> DCHECKs?

Done.

[binji, 2017-02-17 19:12:02]

The CQ bit was checked by binji@chromium.org

[binji, 2017-02-17 19:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2697013009/#ps20001
(title: "merge master + fixes")

[commit-bot: I haz the power, 2017-02-17 19:12:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 19:17:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 19:17:41]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[binji, 2017-02-22 20:27:34]

The CQ bit was checked by binji@chromium.org

[binji, 2017-02-22 20:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2697013009/#ps40001
(title: "merge master + USE variable that's only used in DCHECK")

[commit-bot: I haz the power, 2017-02-22 20:27:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:47:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:47:30]

Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/23075)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[binji, 2017-02-22 22:05:56]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 22:06:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:39:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 22:39:22]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[binji, 2017-02-24 20:16:45]

The CQ bit was checked by binji@chromium.org

[binji, 2017-02-24 20:16:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2697013009/#ps100001
(title: "remove change to coverage-expected")

[commit-bot: I haz the power, 2017-02-24 20:16:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 20:40:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 20:40:58]

Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[binji, 2017-02-28 20:03:17]

The CQ bit was checked by binji@chromium.org

[binji, 2017-02-28 20:03:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2697013009/#ps120001
(title: "merge master")

[commit-bot: I haz the power, 2017-02-28 20:03:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 20:31:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488312197413580,
"parent_rev": "9c385f0405bb7d91259632f517c2c75e8dbb485b", "commit_rev":
"cb8fb46aa3f1bc785f89bc16ad389f7a9b4fe982"}

[commit-bot: I haz the power, 2017-02-28 20:31:09]

Description was changed from

==========
Move ArrayBuffer.prototype.slice implementation to C++

This will be useful for sharing the implementation with
SharedArrayBuffer.prototype.slice.

BUG=v8:5897
==========

to

==========
Move ArrayBuffer.prototype.slice implementation to C++

This will be useful for sharing the implementation with
SharedArrayBuffer.prototype.slice.

BUG=v8:5897

Review-Url: https://codereview.chromium.org/2697013009
Cr-Commit-Position: refs/heads/master@{#43503}
Committed:
https://chromium.googlesource.com/v8/v8/+/cb8fb46aa3f1bc785f89bc16ad389f7a9b4...
==========

[commit-bot: I haz the power, 2017-02-28 20:31:10]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/cb8fb46aa3f1bc785f89bc16ad389f7a9b4...