Object.observe: include oldValue in change records, (issue 11362115)

[ross...@chromium.org]

Reviewers: Toon Verwaest,

Description:
Object.observe: include oldValue in change records,
plus more accurate distinction of different change types.

Required handlifying more code.

Also fixed a handlification bug in JSProxy::GetElementAttributeWithHandler.

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


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

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

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

[ad...@chromium.org]

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

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode2911
src/objects.cc:2911: case INTERCEPTOR:
Asking for oldValues for interceptors (or in fact even notifying about
changes to intercepted properties) is going to be a little surprising
(though possibly useful). I didn't notice that the last change handled
this. I believe this means that

Object.observe(document.body.style, ...)
document.body.style.background = 'something'

will emit a record that includes the old value of background. But of
course we won't be notified of any style changes that happen outside of
script. Is this really what we want?

https://codereview.chromium.org/11362115/

[verw...@chromium.org]

lgtm with comments

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

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode2971
src/objects.cc:2971: result = ConvertDescriptorToField(*name, *value,
attributes);
self->

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode2984
src/objects.cc:2984: result = ConvertTransitionToMapTransition(
self->

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode3009
src/objects.cc:3009: self->EnqueueChangeRecord("updated", name,
old_value);
Wouldn't it be less error-prone to make EnqueueChangeRecord a static
method that takes a handle in for the receiver?

https://codereview.chromium.org/11362115/

[raf...@chromium.org]

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

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode2913
src/objects.cc:2913: Object::GetProperty(self, self, lookup, name,
&old_attributes);
Consider using lookup.GetLazyValue()? Note: this would bypass
interceptors.

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode3002
src/objects.cc:3002: } else if (new_attributes != old_attributes) {
out of curiosity: how can it happen that new_attributes !=
old_attributes?

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode3091
src/objects.cc:3091: Object::GetProperty(self, self, &lookup, name,
&old_attributes);
ditto: GetLazyValue()

https://codereview.chromium.org/11362115/

[ad...@chromium.org]

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

https://codereview.chromium.org/11362115/diff/1/src/objects.cc#newcode2913
src/objects.cc:2913: Object::GetProperty(self, self, lookup, name,
&old_attributes);

On 2012/11/07 10:26:20, rafaelw wrote:
> Consider using lookup.GetLazyValue()? Note: this would bypass
interceptors.

Might also bypass access checks, though, which I don't think is right...

https://codereview.chromium.org/11362115/

[ross...@chromium.org]

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

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode2911
src/objects.cc:2911: case INTERCEPTOR:

On 2012/11/06 14:08:53, adamk wrote:
> Asking for oldValues for interceptors (or in fact even notifying about
changes
> to intercepted properties) is going to be a little surprising (though
possibly
> useful). I didn't notice that the last change handled this. I believe
this means
> that

> Object.observe(document.body.style, ...)
> document.body.style.background = 'something'

> will emit a record that includes the old value of background. But of
course we
> won't be notified of any style changes that happen outside of script.
Is this
> really what we want?

Changed.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode2913

src/objects.cc:2913: Object::GetProperty(self, self, lookup, name,
&old_attributes);

On 2012/11/07 10:30:29, adamk wrote:
> On 2012/11/07 10:26:20, rafaelw wrote:
> > Consider using lookup.GetLazyValue()? Note: this would bypass
interceptors.

> Might also bypass access checks, though, which I don't think is
right...

Access checks are already handled further up, I think.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode2913

src/objects.cc:2913: Object::GetProperty(self, self, lookup, name,
&old_attributes);
On 2012/11/07 10:26:20, rafaelw wrote:
> Consider using lookup.GetLazyValue()? Note: this would bypass
interceptors.

Done, here and elsewhere. With a little tweak to that function
(returning the hole in the default case), it also simplified some of the
code quite a bit.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode2971

src/objects.cc:2971: result = ConvertDescriptorToField(*name, *value,
attributes);

On 2012/11/06 17:35:47, Toon Verwaest wrote:
> self->

Done.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode2984

src/objects.cc:2984: result = ConvertTransitionToMapTransition(

On 2012/11/06 17:35:47, Toon Verwaest wrote:
> self->

Done.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode3002

src/objects.cc:3002: } else if (new_attributes != old_attributes) {

On 2012/11/07 10:26:20, rafaelw wrote:

> out of curiosity: how can it happen that new_attributes !=
old_attributes?

Right, I think it cannot in this case. Removed.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode3009

src/objects.cc:3009: self->EnqueueChangeRecord("updated", name,
old_value);

On 2012/11/06 17:35:47, Toon Verwaest wrote:
> Wouldn't it be less error-prone to make EnqueueChangeRecord a static
method that
> takes a handle in for the receiver?

Probably, but the same is true for quite a few other methods. In the
face of that, I favour consistency, at least for the time being.

https://chromiumcodereview.appspot.com/11362115/diff/1/src/objects.cc#newcode3091
src/objects.cc:3091: Object::GetProperty(self, self, &lookup, name,

&old_attributes);
On 2012/11/07 10:26:20, rafaelw wrote:

> ditto: GetLazyValue()

Done.

https://chromiumcodereview.appspot.com/11362115/