Object.observe: include oldValue in change records,

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=verwaest@chromium.org
BUG=

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

[adamk, 2012-11-06 14:08:53]

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?

[Toon Verwaest, 2012-11-06 17:35:47]

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?

[rafaelw, 2012-11-07 10:26:20]

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()

[adamk, 2012-11-07 10:30:29]

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

[rossberg, 2012-11-07 14:05:54]

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#newcode...
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#newcode...
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#newcode...
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#newcode...
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#newcode...
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#newcode...
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#newcode...
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#newcode...
src/objects.cc:3091: Object::GetProperty(self, self, &lookup, name,
&old_attributes);
On 2012/11/07 10:26:20, rafaelw wrote:
> ditto: GetLazyValue()

Done.