Object.observe: generate change records for indexed properties. (issue 11365111)

[ross...@chromium.org]

Reviewers: Toon Verwaest,

Description:
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.

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

R=verw...@chromium.org
BUG=


Please review this at https://codereview.chromium.org/11365111/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/elements.h
M src/elements.cc
M src/objects-inl.h
M src/objects.h
M src/objects.cc
M test/mjsunit/harmony/object-observe.js

[verw...@chromium.org]

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.

http://codereview.chromium.org/11365111/

[mstar...@chromium.org]

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.

https://codereview.chromium.org/11365111/

[ross...@chromium.org]

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#newcode585
src/elements.cc:585: ? NONE : ABSENT;
That was wrong.

https://chromiumcodereview.appspot.com/11365111/diff/1/src/elements.cc#newcode1472

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#newcode1545

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#newcode1549

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#newcode4074

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#newcode10230

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.

https://chromiumcodereview.appspot.com/11365111/

[mstar...@chromium.org]

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.

https://codereview.chromium.org/11365111/

[mstar...@chromium.org]

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.

https://codereview.chromium.org/11365111/