Change Object.defineProperty to accept undefined as getters and setters and t...

Change Object.defineProperty to accept undefined as getters and setters and to correctly accept overriding an accessor with a data property.

In the past we only accepted functions as argument for setting an
accessor. Since one should be able to set an accessor to undefined
this had to be changed to take either.

In addition, we did not lookup properties in the prototype chain,
causing us to call the setter of an existing accessor up the prototype
chain when trying to replace an existing accessor (that was not local)
with a data property.



Committed: http://code.google.com/p/v8/source/detail?r=6045

[Rico, 2010-12-16 09:16:54]

This fixes issue 687

[Lasse Reichstein, 2010-12-16 09:43:34]

LGTM

http://codereview.chromium.org/5861006/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/5861006/diff/5001/src/runtime.cc#newcode3517
src/runtime.cc:3517: CONVERT_CHECKED(String, name, args[1]);
This argument is not stored in a handle, so why use a handle for fun?
(In fact, this whole function seems like it should not use handles at all, since
it doesn't for most of its values).

http://codereview.chromium.org/5861006/diff/5001/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5861006/diff/5001/src/v8natives.js#newcode626
src/v8natives.js:626: if (desc.hasGetter() &&
How about putting desc.getGet() in a variable (and ditto for desc.getSet()
below). You are up to two or three uses of it by now.

http://codereview.chromium.org/5861006/diff/5001/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-687.js (right):

http://codereview.chromium.org/5861006/diff/5001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-687.js:45: set value(v) {
Object.defineProperty(this, "value", {value: v}); }};
Indentation.

[Rico, 2010-12-16 12:17:55]

http://codereview.chromium.org/5861006/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/5861006/diff/5001/src/runtime.cc#newcode3517
src/runtime.cc:3517: CONVERT_CHECKED(String, name, args[1]);
On 2010/12/16 09:43:34, Lasse Reichstein wrote:
> This argument is not stored in a handle, so why use a handle for fun?
> (In fact, this whole function seems like it should not use handles at all,
since
> it doesn't for most of its values).

Done.

http://codereview.chromium.org/5861006/diff/5001/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5861006/diff/5001/src/v8natives.js#newcode626
src/v8natives.js:626: if (desc.hasGetter() &&
On 2010/12/16 09:43:34, Lasse Reichstein wrote:
> How about putting desc.getGet() in a variable (and ditto for desc.getSet()
> below). You are up to two or three uses of it by now.
Well, we don't know if desc has a getter, intuitively I would not like to have a
variable with a value that is not there (if you call getGet it will return
undefined even when there is no getter)

http://codereview.chromium.org/5861006/diff/5001/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-687.js (right):

http://codereview.chromium.org/5861006/diff/5001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-687.js:45: set value(v) {
Object.defineProperty(this, "value", {value: v}); }};
On 2010/12/16 09:43:34, Lasse Reichstein wrote:
> Indentation.

Done.

http://codereview.chromium.org/5861006/diff/5001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-687.js:75: 
Removed the rest of this test file, the stuff below was a regression test for
issue 992