Object.observe: generate change records for indexed properties.

Object.observe: generate change records for indexed properties.

Details:
- Extend ElementAccessors with GetAttributes method.
- Add HasLocalElement, Get[Local]ElementAttribute methods to JSReceiver/JSObject.
- Otherwise, mirror implementation for named properties.

Cannot correctly handle the cases yet where an accessor is redefined or deleted.

Also fixed handling of object info table.

(Based on CL https://codereview.chromium.org/11362115/)

R=verwaest@chromium.org,mstarzinger@chromium.org
BUG=

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

[Toon Verwaest, 2012-11-07 10:06:52]

lgtm with nits

http://codereview.chromium.org/11365111/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/11365111/diff/1/src/objects.cc#newcode4074
src/objects.cc:4074: if (self->HasIndexedInterceptor()) {
Maybe use (self->HasIndexedInterceptor() && mode != FORCE_DELETION) to reduce
the number of paths. There is no need to set the mode to FORCE_DELETION if it is
already FORCE_DELETION.

Otherwise put { } around if/else branches.

http://codereview.chromium.org/11365111/diff/1/src/objects.cc#newcode10230
src/objects.cc:10230: MaybeObject* result = *value;
This initialization seems superfluous since it's overwritten directly
afterwards.

[Michael Starzinger, 2012-11-07 11:47:48]

https://codereview.chromium.org/11365111/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11365111/diff/1/src/elements.cc#newcode1472
src/elements.cc:1472: if (entry != SeededNumberDictionary::kNotFound)
Curly brackets around body.

https://codereview.chromium.org/11365111/diff/1/src/elements.cc#newcode1545
src/elements.cc:1545: Object* probe = key < (length - 2) ?
parameter_map->get(key + 2) : NULL;
Better use the GetParameterMapArg() helper for that to not duplicate this magic
throughout the code.

https://codereview.chromium.org/11365111/diff/1/src/elements.cc#newcode1549
src/elements.cc:1549: if (arguments->IsDictionary()) {
Better delegate to the elements accessor for the underlying backing store.

[rossberg, 2012-11-07 18:41:42]

PTAL. There was a bug in the handling of intercepted indexed properties and the
prototype chain, that a cctest uncovered. Unfortunately, the proper fix required
implementing yet more functionality.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/11365111/diff/1/src/elements.cc#newcod...
src/elements.cc:585: ? NONE : ABSENT;
That was wrong.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/elements.cc#newcod...
src/elements.cc:1472: if (entry != SeededNumberDictionary::kNotFound)
On 2012/11/07 11:47:48, Michael Starzinger wrote:
> Curly brackets around body.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/elements.cc#newcod...
src/elements.cc:1545: Object* probe = key < (length - 2) ?
parameter_map->get(key + 2) : NULL;
On 2012/11/07 11:47:48, Michael Starzinger wrote:
> Better use the GetParameterMapArg() helper for that to not duplicate this
magic
> throughout the code.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/elements.cc#newcod...
src/elements.cc:1549: if (arguments->IsDictionary()) {
On 2012/11/07 11:47:48, Michael Starzinger wrote:
> Better delegate to the elements accessor for the underlying backing store.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/11365111/diff/1/src/objects.cc#newcode...
src/objects.cc:4074: if (self->HasIndexedInterceptor()) {
On 2012/11/07 10:06:52, Toon Verwaest wrote:
> Maybe use (self->HasIndexedInterceptor() && mode != FORCE_DELETION) to reduce
> the number of paths. There is no need to set the mode to FORCE_DELETION if it
is
> already FORCE_DELETION.
> 
> Otherwise put { } around if/else branches.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/objects.cc#newcode...
src/objects.cc:10230: MaybeObject* result = *value;
On 2012/11/07 10:06:52, Toon Verwaest wrote:
> This initialization seems superfluous since it's overwritten directly
> afterwards.

Done.

[Michael Starzinger, 2012-11-07 19:43:49]

LGTM with two final nits. Note that I only looked at the elements accessors.

https://codereview.chromium.org/11365111/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11365111/diff/1/src/elements.cc#newcode585
src/elements.cc:585: ? NONE : ABSENT;
On 2012/11/07 18:41:42, rossberg wrote:
> That was wrong.

Nice catch.

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc#newcode584
src/elements.cc:584: if (key >=
ElementsAccessorSubclass::GetCapacityImpl(backing_store))
Curly brackets around body.

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc#newcode1543
src/elements.cc:1543: // Aliased parameters and non-aliased elements in a fast
backing store
I think we can drop all three lines of comments. It adds no valuable information
here. At least this is the wrong place to describe our crazy arguments object
layout.

[Michael Starzinger, 2012-11-07 19:54:03]

Another turbo-nit.

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc#newcode1168
src/elements.cc:1168: 
Drop one of the two newlines.

[Toon Verwaest, 2012-11-08 10:01:31]

lgtm with comments

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc#newcode586
src/elements.cc:586: return backing_store->get(key)->IsTheHole() ? ABSENT :
NONE;
You can probably use is_the_hole(key) here.

https://codereview.chromium.org/11365111/diff/5002/src/elements.cc#newcode1551
src/elements.cc:1551: FixedArray* arguments =
FixedArray::cast(parameter_map->get(1));
Can we use a constant here? (kArgumentsIndex or so)

[rossberg, 2012-11-08 11:22:56]

Maybe you want to have another quick look -- the latest diff mostly removes
code. :)

I entirely removed the previous implementations of 2 largish HasElement...
methods and delegate to the new respective GetElementAttribute instead. It's now
simpler and more regular.

Also renamed the ad-hoc version of the JSObject::HasLocalElement method not
actually returning a bool to GetLocalElementType. Now only used where really
needed.

https://chromiumcodereview.appspot.com/11365111/diff/5002/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/11365111/diff/5002/src/elements.cc#new...
src/elements.cc:584: if (key >=
ElementsAccessorSubclass::GetCapacityImpl(backing_store))
On 2012/11/07 19:43:50, Michael Starzinger wrote:
> Curly brackets around body.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/5002/src/elements.cc#new...
src/elements.cc:586: return backing_store->get(key)->IsTheHole() ? ABSENT :
NONE;
On 2012/11/08 10:01:31, Toon Verwaest wrote:
> You can probably use is_the_hole(key) here.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/5002/src/elements.cc#new...
src/elements.cc:1168: 
On 2012/11/07 19:54:03, Michael Starzinger wrote:
> Drop one of the two newlines.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/5002/src/elements.cc#new...
src/elements.cc:1543: // Aliased parameters and non-aliased elements in a fast
backing store
On 2012/11/07 19:43:50, Michael Starzinger wrote:
> I think we can drop all three lines of comments. It adds no valuable
information
> here. At least this is the wrong place to describe our crazy arguments object
> layout.

Done.

https://chromiumcodereview.appspot.com/11365111/diff/5002/src/elements.cc#new...
src/elements.cc:1551: FixedArray* arguments =
FixedArray::cast(parameter_map->get(1));
On 2012/11/08 10:01:31, Toon Verwaest wrote:
> Can we use a constant here? (kArgumentsIndex or so)

Just mirrored this after GetImpl, which does the same thing. :)

[Toon Verwaest, 2012-11-08 11:58:41]

lgtm