Object.observe support for Function 'prototype' property

Object.observe support for Function 'prototype' property

BUG=v8:2409
Committed: http://code.google.com/p/v8/source/detail?r=13177

[rossberg, 2012-12-05 12:02:28]

Mostly looks good, but I have questions.

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

https://codereview.chromium.org/11416353/diff/1/src/objects.cc#newcode3115
src/objects.cc:3115: if (!maybe->ToHandle(&old_value, isolate)) return maybe;
I don't understand why this works. How come you don't have to set old_attributes
in this case?

https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:822: assertSame(fun,
observer.records[0].object);
What is the reason you check the first record outside assertCallbackRecords?

[adamk, 2012-12-05 17:38:17]

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

https://codereview.chromium.org/11416353/diff/1/src/objects.cc#newcode3115
src/objects.cc:3115: if (!maybe->ToHandle(&old_value, isolate)) return maybe;
On 2012/12/05 12:02:29, rossberg wrote:
> I don't understand why this works. How come you don't have to set
old_attributes
> in this case?

Hmm, I don't understand either. Added a test that should tickle it, but doesn't.
Moved the attributes-setting outside the inner if statement for now, but would
like to figure out a test that would result in different behavior...

https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:822: assertSame(fun,
observer.records[0].object);
On 2012/12/05 12:02:29, rossberg wrote:
> What is the reason you check the first record outside assertCallbackRecords?

Because of line 827: the only handle to the oldValue is in the record itself, so
the thing I compare is oldValue.constructor instead of simply oldValue.

[adamk, 2012-12-05 19:47:38]

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

https://codereview.chromium.org/11416353/diff/1/src/objects.cc#newcode3115
src/objects.cc:3115: if (!maybe->ToHandle(&old_value, isolate)) return maybe;
On 2012/12/05 17:38:17, adamk wrote:
> On 2012/12/05 12:02:29, rossberg wrote:
> > I don't understand why this works. How come you don't have to set
> old_attributes
> > in this case?
> 
> Hmm, I don't understand either. Added a test that should tickle it, but
doesn't.
> Moved the attributes-setting outside the inner if statement for now, but would
> like to figure out a test that would result in different behavior...

Ah, here's why: there's that special case in %DefineOrRedefineDataProperty when
the lookup result is a callback property and the attributes aren't being
changed. So this code is run only when attributes are changing. In any case, I
think always setting old_attributes is more semantically correct.

And hopefully the TODO in %DefineOrRedefineDataProperty will be fixed and this
code can be removed.

[rossberg, 2012-12-06 09:49:41]

On 2012/12/05 19:47:38, adamk wrote:
> On 2012/12/05 17:38:17, adamk wrote:
> > On 2012/12/05 12:02:29, rossberg wrote:
> > > I don't understand why this works. How come you don't have to
> > > set old_attributes in this case?
> > 
> > Hmm, I don't understand either. Added a test that should tickle
> > it, but doesn't.
> > Moved the attributes-setting outside the inner if statement for now, but
would
> > like to figure out a test that would result in different behavior...
> 
> Ah, here's why: there's that special case in %DefineOrRedefineDataProperty
when
> the lookup result is a callback property and the attributes aren't being
> changed. So this code is run only when attributes are changing.

Sigh, what a mess...

[rossberg, 2012-12-06 09:50:01]

https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:822: assertSame(fun,
observer.records[0].object);
On 2012/12/05 17:38:17, adamk wrote:
> On 2012/12/05 12:02:29, rossberg wrote:
> > What is the reason you check the first record outside assertCallbackRecords?
> 
> Because of line 827: the only handle to the oldValue is in the record itself,
so
> the thing I compare is oldValue.constructor instead of simply oldValue.

Well, you can save the old fun.prototype before replacing it in line 844. ;)

[adamk, 2012-12-06 17:31:57]

On 2012/12/06 09:50:01, rossberg wrote:
>
https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
> File test/mjsunit/harmony/object-observe.js (right):
> 
>
https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
> test/mjsunit/harmony/object-observe.js:822: assertSame(fun,
> observer.records[0].object);
> On 2012/12/05 17:38:17, adamk wrote:
> > On 2012/12/05 12:02:29, rossberg wrote:
> > > What is the reason you check the first record outside
assertCallbackRecords?
> > 
> > Because of line 827: the only handle to the oldValue is in the record
itself,
> so
> > the thing I compare is oldValue.constructor instead of simply oldValue.
> 
> Well, you can save the old fun.prototype before replacing it in line 844. ;)

Ah, but it doesn't exist yet: it's lazily created in
Accessors::FunctionGetPrototype(). That's the point of this test-case: to
exercise my code in FunctionSetPrototype() that lazily creates the "old value"
in the "right way" (with Heap::AllocateFunctionPrototype). It's not just a
generic Object.

[rossberg, 2012-12-07 10:09:57]

On 2012/12/06 17:31:57, adamk wrote:
> On 2012/12/06 09:50:01, rossberg wrote:
> >
>
https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
> > File test/mjsunit/harmony/object-observe.js (right):
> > 
> >
>
https://codereview.chromium.org/11416353/diff/1/test/mjsunit/harmony/object-o...
> > test/mjsunit/harmony/object-observe.js:822: assertSame(fun,
> > observer.records[0].object);
> > On 2012/12/05 17:38:17, adamk wrote:
> > > On 2012/12/05 12:02:29, rossberg wrote:
> > > > What is the reason you check the first record outside
> assertCallbackRecords?
> > > 
> > > Because of line 827: the only handle to the oldValue is in the record
> itself,
> > so
> > > the thing I compare is oldValue.constructor instead of simply oldValue.
> > 
> > Well, you can save the old fun.prototype before replacing it in line 844. ;)
> 
> Ah, but it doesn't exist yet: it's lazily created in
> Accessors::FunctionGetPrototype(). That's the point of this test-case: to
> exercise my code in FunctionSetPrototype() that lazily creates the "old value"
> in the "right way" (with Heap::AllocateFunctionPrototype). It's not just a
> generic Object.

Ah, I see. That makes sense. You may want to say that a bit more explicitly in
the comment. I for one did not get the point. :)

[rossberg, 2012-12-10 10:48:20]

LGTM, landing...