Object.observe: Handle oldValue for elements with accessors properly. (issue 11358234)

[ross...@chromium.org]

Reviewers: Michael Starzinger,

Description:
Object.observe: Handle oldValue for elements with accessors properly.

Extended ElementAccessor interface to allow querying PropertyType and
AccessorPair. Also added respective functionality to JSObject.

R=mstar...@chromium.org
BUG=


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

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

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

[mstar...@chromium.org]

LGTM with two comments.


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

https://codereview.chromium.org/11358234/diff/1/src/elements.cc#newcode1551
src/elements.cc:1551: return
AccessorPair::cast(backing_store->ValueAt(entry));
This could potentially also be a Foreign callback or a AccessorInfo
callback. I am unsure if that ever happens in any of our embedders, so
we could just add a CHECK (on top of the implicit assertion) here that
the value we get out actually is a AccessorPair. WDYT?

https://codereview.chromium.org/11358234/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11358234/diff/1/src/objects.h#newcode1844
src/objects.h:1844: enum LocalElementKind {
As discussed offline, we should try to get rid of this enum completely.
But until then I am fine with the new name.

https://codereview.chromium.org/11358234/

[sven...@chromium.org]

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

https://codereview.chromium.org/11358234/diff/1/src/elements.cc#newcode1551
src/elements.cc:1551: return
AccessorPair::cast(backing_store->ValueAt(entry));

On 2012/11/14 17:30:39, Michael Starzinger wrote:
> This could potentially also be a Foreign callback or a AccessorInfo
callback. I
> am unsure if that ever happens in any of our embedders, so we could
just add a
> CHECK (on top of the implicit assertion) here that the value we get
out actually
> is a AccessorPair. WDYT?

The Foreign case is completely under our control and (very probably)
doesn't matter here, but the unhandled AccessorInfo case really makes me
nervous: If I understand things correctly, this would change our
external API with unknown consequences. I would very much prefer
avoiding this. If there is really, really no way around this, the
restriction (which one exactly?) should be documented in a very visible
way in our external header.

https://codereview.chromium.org/11358234/

[ross...@chromium.org]

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

https://codereview.chromium.org/11358234/diff/1/src/elements.cc#newcode1551
src/elements.cc:1551: return
AccessorPair::cast(backing_store->ValueAt(entry));

On 2012/11/15 06:36:32, Sven Panne wrote:
> On 2012/11/14 17:30:39, Michael Starzinger wrote:
> > This could potentially also be a Foreign callback or a AccessorInfo
callback.
> I
> > am unsure if that ever happens in any of our embedders, so we could
just add a
> > CHECK (on top of the implicit assertion) here that the value we get
out
> actually
> > is a AccessorPair. WDYT?

> The Foreign case is completely under our control and (very probably)
doesn't
> matter here, but the unhandled AccessorInfo case really makes me
nervous: If I
> understand things correctly, this would change our external API with
unknown
> consequences. I would very much prefer avoiding this. If there is
really, really
> no way around this, the restriction (which one exactly?) should be
documented in
> a very visible way in our external header.

That was an oversight. Added IsAccessorPair condition, returning NULL
otherwise (analogous to JSObject::GetLocalPropertyAccessorPair).

https://codereview.chromium.org/11358234/