Object.observe: don't unnecessarily emit oldValue for reconfigurations of data properties

Object.observe: don't unnecessarily emit oldValue for reconfigurations of data properties

When a data property has its attributes changed but its value remains the same,
don't emit an oldValue. This makes the API more consistent by only emitting
oldValue when the value of a property has actually changed (or been removed,
in the case of a reconfiguration as an accessor property or a deletion).

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

[rossberg, 2013-01-17 16:57:44]

LGTM, with nit.

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

https://codereview.chromium.org/11820004/diff/1/src/objects.cc#newcode3189
src/objects.cc:3189: } else if (old_value->IsTheHole()) {
Is this extra branch actually needed? It seems like the SameValue check below
should apply just fine, and you can keep the condition in the 'if' below.

https://codereview.chromium.org/11820004/diff/1/src/objects.cc#newcode10262
src/objects.cc:10262: } else if (old_value->IsTheHole()) {
Similarly here.

[adamk, 2013-01-22 22:30:10]

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

https://codereview.chromium.org/11820004/diff/1/src/objects.cc#newcode3189
src/objects.cc:3189: } else if (old_value->IsTheHole()) {
On 2013/01/17 16:57:44, rossberg wrote:
> Is this extra branch actually needed? It seems like the SameValue check below
> should apply just fine, and you can keep the condition in the 'if' below.

I think I pulled this out to avoid an unnecessary GetProperty() call for the
oldValue-is-the-hole case, and to make it clear that the only thing triggering
this code path is the absence of the old value. Is your objection to the "extra"
call to EnqueueChangeRecord?

[rossberg, 2013-01-28 13:30:43]

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

https://codereview.chromium.org/11820004/diff/1/src/objects.cc#newcode3189
src/objects.cc:3189: } else if (old_value->IsTheHole()) {
On 2013/01/22 22:30:10, adamk wrote:
> On 2013/01/17 16:57:44, rossberg wrote:
> > Is this extra branch actually needed? It seems like the SameValue check
below
> > should apply just fine, and you can keep the condition in the 'if' below.
> 
> I think I pulled this out to avoid an unnecessary GetProperty() call for the
> oldValue-is-the-hole case, and to make it clear that the only thing triggering
> this code path is the absence of the old value. Is your objection to the
"extra"
> call to EnqueueChangeRecord?

Yeah, the fewer code paths the better, but I don't feel too strong about it in
this case. I leave it up to you.