Fix %GetArrayKeys to not skip non-enumerable indices

Fix %GetArrayKeys to not skip non-enumerable indices

This is one step in the direction of fixing a range of small bugs in the array methods when dealing with non-standard element attributes.

Added tests exercising this behavior for shift and unshift.

For Proxies and Interceptors, the behavior of %GetArrayKeys is now to just return an interval, rather than trying to list all their indexed properties. In the Proxy case, this seems like the only way to avoid an observable difference between smart and non-smart array methods. For Interceptors, the usual case (in WebKit, anyway) is for them to have all indices in [0, length), so enumerating them won't be any better than simply iterating over that range.

Committed: https://code.google.com/p/v8/source/detail?r=14057

[adamk, 2013-03-21 21:58:20]

Let me know if I should file a tracking bug for this work (I plan there to be
several more patches in this series). The basic idea is to tighten up the array
method behavior in preparation for creating a new Object.observe mutation type
('splice') as discussed at last week's TC39 meeting.

[Michael Starzinger, 2013-03-22 10:12:20]

Drive-by comment. I'll leave the rest of this CL to Andreas. :)

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc#newcode9543
src/runtime.cc:9543: static Object* NewSingleInterval(Isolate* isolate, uint32_t
length) {
It is dangerous to have a method returning an unhandlified value, that has a
handlified implementation. Better return Handle<Object> here.

[rossberg, 2013-03-22 14:22:34]

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc#newcode9579
src/runtime.cc:9579: int keys_length = keys->length();
Isn't the filtering below redundant now? You only collect elements anyway.

[adamk, 2013-03-22 15:11:18]

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc#newcode9543
src/runtime.cc:9543: static Object* NewSingleInterval(Isolate* isolate, uint32_t
length) {
On 2013/03/22 10:12:20, Michael Starzinger wrote:
> It is dangerous to have a method returning an unhandlified value, that has a
> handlified implementation. Better return Handle<Object> here.

Fair point. Going back and forth between JS and C++ makes me wish for
function-scoped functions in the latter...

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc#newcode9579
src/runtime.cc:9579: int keys_length = keys->length();
On 2013/03/22 14:22:34, rossberg wrote:
> Isn't the filtering below redundant now? You only collect elements anyway.

I thought so too, but a test caught me: still need to filter out things >=
length. Or I guess I could do that in JS, would that be a better approach?

[rossberg, 2013-03-22 17:27:22]

LGTM with comments

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc#newcode9579
src/runtime.cc:9579: int keys_length = keys->length();
On 2013/03/22 15:11:18, adamk wrote:
> On 2013/03/22 14:22:34, rossberg wrote:
> > Isn't the filtering below redundant now? You only collect elements anyway.
> 
> I thought so too, but a test caught me: still need to filter out things >=
> length. Or I guess I could do that in JS, would that be a better approach?

Oh dear. Well, since the spec of the function says so, it probably has to be
done here. But you can still remove the first half of the condition below. And
maybe add a brief comment about what's being filtered.

[adamk, 2013-03-22 17:51:34]

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12653010/diff/2001/src/runtime.cc#newcode9579
src/runtime.cc:9579: int keys_length = keys->length();
On 2013/03/22 17:27:22, rossberg wrote:
> On 2013/03/22 15:11:18, adamk wrote:
> > On 2013/03/22 14:22:34, rossberg wrote:
> > > Isn't the filtering below redundant now? You only collect elements anyway.
> > 
> > I thought so too, but a test caught me: still need to filter out things >=
> > length. Or I guess I could do that in JS, would that be a better approach?
> 
> Oh dear. Well, since the spec of the function says so, it probably has to be
> done here. But you can still remove the first half of the condition below. And
> maybe add a brief comment about what's being filtered.

Added a comment, and a TODO (as I plan to change this function to be more
sensible).

[adamk, 2013-03-22 18:04:36]

Committed patchset #2 manually as r14057 (presubmit successful).