Throw as per spec when modifying an Array with builtin methods

Throw as per spec when modifying an Array with builtin methods

BUG=v8:3684
LOG=n

Committed: https://chromium.googlesource.com/v8/v8/+/6d1d522e55fa53feb248aad652eb9275d9300844

[adamk, 2014-11-13 23:43:28]

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

[adamk, 2014-11-13 23:43:29]

https://codereview.chromium.org/726773002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/726773002/diff/1/src/objects.cc#newcode13100
src/objects.cc:13100: bool JSArray::HasReadOnlyLength(Handle<JSArray> array) {
Should this just delegate to IsReadOnlyLengthDescriptor above? It's not clear to
me why the code in the two methods looks different...

https://codereview.chromium.org/726773002/diff/1/test/mjsunit/array-methods-r...
File test/mjsunit/array-methods-read-only-length.js (right):

https://codereview.chromium.org/726773002/diff/1/test/mjsunit/array-methods-r...
test/mjsunit/array-methods-read-only-length.js:154: assertFalse(2 in b);
These have since been fixed.

[Michael Starzinger, 2014-11-14 17:00:21]

mstarzinger@chromium.org changed reviewers:
+ verwaest@chromium.org

[Michael Starzinger, 2014-11-14 17:00:23]

LGTM. Added Toon just FYI about the change in the objects.cc file. And Kudos for
tackling all of the conformance issues in the Array builtins, you are my hero.
:)

https://codereview.chromium.org/726773002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/726773002/diff/1/src/objects.cc#newcode13100
src/objects.cc:13100: bool JSArray::HasReadOnlyLength(Handle<JSArray> array) {
On 2014/11/13 23:43:29, adamk wrote:
> Should this just delegate to IsReadOnlyLengthDescriptor above? It's not clear
to
> me why the code in the two methods looks different...

Acknowledged. I chatted with Toon about this, semantically these two methods
should do the same. But since the LookupIterator is the "better implementation",
we should keep this method and instead deprecate the other one. So I would just
keep it as it is for now (i.e. keep both methods) and maybe even add a TODO to
IsReadOnlyLengthDescriptor that it is deprecated.

https://codereview.chromium.org/726773002/diff/1/test/mjsunit/array-methods-r...
File test/mjsunit/array-methods-read-only-length.js (right):

https://codereview.chromium.org/726773002/diff/1/test/mjsunit/array-methods-r...
test/mjsunit/array-methods-read-only-length.js:154: assertFalse(2 in b);
On 2014/11/13 23:43:29, adamk wrote:
> These have since been fixed.

Acknowledged. Awesome.

[adamk, 2014-11-14 19:13:19]

https://codereview.chromium.org/726773002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/726773002/diff/1/src/objects.cc#newcode13100
src/objects.cc:13100: bool JSArray::HasReadOnlyLength(Handle<JSArray> array) {
On 2014/11/14 17:00:22, Michael Starzinger wrote:
> On 2014/11/13 23:43:29, adamk wrote:
> > Should this just delegate to IsReadOnlyLengthDescriptor above? It's not
clear
> to
> > me why the code in the two methods looks different...
> 
> Acknowledged. I chatted with Toon about this, semantically these two methods
> should do the same. But since the LookupIterator is the "better
implementation",
> we should keep this method and instead deprecate the other one. So I would
just
> keep it as it is for now (i.e. keep both methods) and maybe even add a TODO to
> IsReadOnlyLengthDescriptor that it is deprecated.

Thanks for the background, added a TODO.

[adamk, 2014-11-14 19:13:28]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-11-14 19:14:22]

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

[commit-bot: I haz the power, 2014-11-14 19:42:09]

Committed patchset #2 (id:20001)