[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}

Committed: https://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1
Cr-Commit-Position: refs/heads/master@{#33072}

[Benedikt Meurer, 2015-12-03 10:17:04]

Hey Yang,

Here's another cleanup.
Please take a look.

Thanks,
Benedikt

[Camillo Bruni, 2015-12-03 14:58:27]

cbruni@chromium.org changed reviewers:
+ cbruni@chromium.org

[Camillo Bruni, 2015-12-03 14:58:28]

lgtm with comment.

https://codereview.chromium.org/1500543002/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1500543002/diff/1/src/builtins.cc#newcode1772
src/builtins.cc:1772: ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, number_length,
shouldn't the byteLength conversion happen after creating the object? (step 1 in
the spec), you might get the wrong error message first.

[Benedikt Meurer, 2015-12-03 15:24:56]

https://codereview.chromium.org/1500543002/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1500543002/diff/1/src/builtins.cc#newcode1772
src/builtins.cc:1772: ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, number_length,
On 2015/12/03 14:58:28, cbruni wrote:
> shouldn't the byteLength conversion happen after creating the object? (step 1
in
> the spec), you might get the wrong error message first.

No, for builtins the creation happens later.

[Benedikt Meurer, 2015-12-03 15:25:03]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-12-03 15:25:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/1

[commit-bot: I haz the power, 2015-12-03 15:30:33]

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

[commit-bot: I haz the power, 2015-12-03 15:30:34]

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

[Benedikt Meurer, 2015-12-03 18:53:20]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-12-03 18:53:20]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/1500543002/#ps20001
(title: "REBASE")

[commit-bot: I haz the power, 2015-12-03 18:53:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/20001

[commit-bot: I haz the power, 2015-12-03 21:15:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-03 21:16:39]

Description was changed from

==========
[es6] Correctify and unify ArrayBuffer and SharedArrayBuffer constructors.

The ArrayBuffer and SharedArrayBuffer constructors should raise an
exception when called with no arguments or undefined length.  Also
unified the ArrayBuffer and SharedArrayBuffer implementations as C++
builtins, and removed some (now) obsolete runtime entries.

R=yangguo@chromium.org
==========

to

==========
[es6] Correctify and unify ArrayBuffer and SharedArrayBuffer constructors.

The ArrayBuffer and SharedArrayBuffer constructors should raise an
exception when called with no arguments or undefined length.  Also
unified the ArrayBuffer and SharedArrayBuffer implementations as C++
builtins, and removed some (now) obsolete runtime entries.

R=yangguo@chromium.org

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

[commit-bot: I haz the power, 2015-12-03 21:16:40]

Patchset 2 (id:??) landed as
https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}

[adamk, 2015-12-04 01:41:14]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2015-12-04 01:41:14]

Are you sure this change is web-compatible? Neither Safari nor Firefox throws
when passing no arguments to the ArrayBuffer constructor. It seems worrying that
we had code in our tests that depended upon the ability to call the constructor
in such a manner.

I'd also prefer if CLs didn't do both a rewrite like this (from JS to C++)  and
change author-exposed behavior in the same patch.

[Benedikt Meurer, 2015-12-04 03:17:32]

On 2015/12/04 01:41:14, adamk wrote:
> Are you sure this change is web-compatible? Neither Safari nor Firefox throws
> when passing no arguments to the ArrayBuffer constructor. It seems worrying
that
> we had code in our tests that depended upon the ability to call the
constructor
> in such a manner.
> 
> I'd also prefer if CLs didn't do both a rewrite like this (from JS to C++) 
and
> change author-exposed behavior in the same patch.

I talked with Toon about this before and we concluded that it's OK. It's a one
liner to change this back to make ArrayBuffer() allocate an empty one instead if
this breaks the web. We don't know why it was cemented in our tests, but
probably just sloppiness on our side.

Do you feel strongly about restoring the non spec behavior?

[adamk, 2015-12-04 03:37:39]

On 2015/12/04 03:17:32, Benedikt Meurer wrote:
> On 2015/12/04 01:41:14, adamk wrote:
> > Are you sure this change is web-compatible? Neither Safari nor Firefox
throws
> > when passing no arguments to the ArrayBuffer constructor. It seems worrying
> that
> > we had code in our tests that depended upon the ability to call the
> constructor
> > in such a manner.
> > 
> > I'd also prefer if CLs didn't do both a rewrite like this (from JS to C++) 
> and
> > change author-exposed behavior in the same patch.
> 
> I talked with Toon about this before and we concluded that it's OK. It's a one
> liner to change this back to make ArrayBuffer() allocate an empty one instead
if
> this breaks the web. We don't know why it was cemented in our tests, but
> probably just sloppiness on our side.
> 
> Do you feel strongly about restoring the non spec behavior?

It's not that I feel strongly about this particular case. Rather, any change to
observable behavior has the potential to cause issues, and all the more so when
the new behavior doesn't match other browsers (and those other browsers agree).
What I was reacting to was the lack of discussion of compatibility concerns in
your CL description.

FWIW, searching in google3 (on cs/) I see several non-v8 tests that use 'new
ArrayBuffer()', so those tests will now break in Chrome. I suspect that this
change is likely to break a some tests in the wild, too. Now, breaking unit
tests is not "breaking the web". But I think it's important to balance the
advantages of matching the spec vs the pain we cause developers (and users, if
real sites happen to depend on the old behavior).

[Benedikt Meurer, 2015-12-04 03:51:15]

On 2015/12/04 03:37:39, adamk wrote:
> On 2015/12/04 03:17:32, Benedikt Meurer wrote:
> > On 2015/12/04 01:41:14, adamk wrote:
> > > Are you sure this change is web-compatible? Neither Safari nor Firefox
> throws
> > > when passing no arguments to the ArrayBuffer constructor. It seems
worrying
> > that
> > > we had code in our tests that depended upon the ability to call the
> > constructor
> > > in such a manner.
> > > 
> > > I'd also prefer if CLs didn't do both a rewrite like this (from JS to C++)

> > and
> > > change author-exposed behavior in the same patch.
> > 
> > I talked with Toon about this before and we concluded that it's OK. It's a
one
> > liner to change this back to make ArrayBuffer() allocate an empty one
instead
> if
> > this breaks the web. We don't know why it was cemented in our tests, but
> > probably just sloppiness on our side.
> > 
> > Do you feel strongly about restoring the non spec behavior?
> 
> It's not that I feel strongly about this particular case. Rather, any change
to
> observable behavior has the potential to cause issues, and all the more so
when
> the new behavior doesn't match other browsers (and those other browsers
agree).
> What I was reacting to was the lack of discussion of compatibility concerns in
> your CL description.
> 
> FWIW, searching in google3 (on cs/) I see several non-v8 tests that use 'new
> ArrayBuffer()', so those tests will now break in Chrome. I suspect that this
> change is likely to break a some tests in the wild, too. Now, breaking unit
> tests is not "breaking the web". But I think it's important to balance the
> advantages of matching the spec vs the pain we cause developers (and users, if
> real sites happen to depend on the old behavior).

Fair enough. I'll restore the previous behavior. My main motivation here was to
fix the code duplication between the two ArrayBuffers and the useless JS
wrappers. The spec fix was drive-by.

[adamk, 2015-12-04 04:04:26]

On 2015/12/04 03:51:15, Benedikt Meurer wrote:
> On 2015/12/04 03:37:39, adamk wrote:
> > On 2015/12/04 03:17:32, Benedikt Meurer wrote:
> > > On 2015/12/04 01:41:14, adamk wrote:
> > > > Are you sure this change is web-compatible? Neither Safari nor Firefox
> > throws
> > > > when passing no arguments to the ArrayBuffer constructor. It seems
> worrying
> > > that
> > > > we had code in our tests that depended upon the ability to call the
> > > constructor
> > > > in such a manner.
> > > > 
> > > > I'd also prefer if CLs didn't do both a rewrite like this (from JS to
C++)
> 
> > > and
> > > > change author-exposed behavior in the same patch.
> > > 
> > > I talked with Toon about this before and we concluded that it's OK. It's a
> one
> > > liner to change this back to make ArrayBuffer() allocate an empty one
> instead
> > if
> > > this breaks the web. We don't know why it was cemented in our tests, but
> > > probably just sloppiness on our side.
> > > 
> > > Do you feel strongly about restoring the non spec behavior?
> > 
> > It's not that I feel strongly about this particular case. Rather, any change
> to
> > observable behavior has the potential to cause issues, and all the more so
> when
> > the new behavior doesn't match other browsers (and those other browsers
> agree).
> > What I was reacting to was the lack of discussion of compatibility concerns
in
> > your CL description.
> > 
> > FWIW, searching in google3 (on cs/) I see several non-v8 tests that use 'new
> > ArrayBuffer()', so those tests will now break in Chrome. I suspect that this
> > change is likely to break a some tests in the wild, too. Now, breaking unit
> > tests is not "breaking the web". But I think it's important to balance the
> > advantages of matching the spec vs the pain we cause developers (and users,
if
> > real sites happen to depend on the old behavior).
> 
> Fair enough. I'll restore the previous behavior. My main motivation here was
to
> fix the code duplication between the two ArrayBuffers and the useless JS
> wrappers. The spec fix was drive-by.

Thanks, that seems reasonable for now.

[Michael Achenbach, 2015-12-04 10:37:31]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1501673002/ by machenbach@chromium.org.

The reason for reverting is: Blocks the roll:
https://codereview.chromium.org/1497763004/.

[Benedikt Meurer, 2015-12-31 18:50:18]

Description was changed from

==========
[es6] Correctify and unify ArrayBuffer and SharedArrayBuffer constructors.

The ArrayBuffer and SharedArrayBuffer constructors should raise an
exception when called with no arguments or undefined length.  Also
unified the ArrayBuffer and SharedArrayBuffer implementations as C++
builtins, and removed some (now) obsolete runtime entries.

R=yangguo@chromium.org

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

to

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

R=cbruni@chromium.org
BUG=chromium:565917, v8:4592
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

[Benedikt Meurer, 2015-12-31 18:55:17]

Description was changed from

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

R=cbruni@chromium.org
BUG=chromium:565917, v8:4592
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

to

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

[Benedikt Meurer, 2015-12-31 18:56:31]

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

[commit-bot: I haz the power, 2015-12-31 18:56:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/40001

[Benedikt Meurer, 2015-12-31 19:04:17]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2015-12-31 19:04:23]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-12-31 19:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/1500543002/#ps40001
(title: "REBASE. Fix.")

[commit-bot: I haz the power, 2015-12-31 19:04:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/40001

[Benedikt Meurer, 2015-12-31 19:12:22]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2015-12-31 19:32:34]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-12-31 19:32:34]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/1500543002/#ps60001
(title: "Get the exceptions right. Other fixes.")

[commit-bot: I haz the power, 2015-12-31 19:32:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/60001

[commit-bot: I haz the power, 2015-12-31 22:13:29]

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

[commit-bot: I haz the power, 2015-12-31 22:13:30]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Benedikt Meurer, 2016-01-01 05:50:12]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2016-01-01 05:50:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/60001

[Benedikt Meurer, 2016-01-01 07:11:05]

Description was changed from

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

to

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

[Benedikt Meurer, 2016-01-01 07:11:11]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2016-01-01 07:11:18]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2016-01-01 07:11:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500543002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500543002/60001

[commit-bot: I haz the power, 2016-01-01 07:12:54]

Description was changed from

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

to

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

[commit-bot: I haz the power, 2016-01-01 07:12:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-01 07:13:27]

Description was changed from

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}
==========

to

==========
[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=chromium:565917, v8:4592
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}

Committed: https://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1
Cr-Commit-Position: refs/heads/master@{#33072}
==========

[commit-bot: I haz the power, 2016-01-01 07:13:28]

Patchset 4 (id:??) landed as
https://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1
Cr-Commit-Position: refs/heads/master@{#33072}

[Benedikt Meurer, 2016-01-01 07:14:04]

Linux Chromium NVIDIA GPU tests have trouble upstream; Chromium waterfall green
nevertheless.