Fix issue 965.

Fix issue 965.

[Rico, 2010-12-10 10:42:35]

LGTM

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js#newcode618
src/v8natives.js:618: var value =
Maybe add a comment that states that this actually works even when no existing
value is present (since it defaults to undefined) - even when this is an
accessor.

http://codereview.chromium.org/5773002/diff/1/test/mjsunit/object-define-prop...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/5773002/diff/1/test/mjsunit/object-define-prop...
test/mjsunit/object-define-property.js:880: 
Maybe add an additional test that makes sure that an accessor is actually
overwritten correctly (step 9.c) if we only set the writable flag

[Kevin Millikin (Chromium), 2010-12-10 11:08:23]

Drive by.

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js#newcode618
src/v8natives.js:618: var value =
I would actually suggest writing it just like the rest of the code in this
function: if (desc.hasValue()) { ... } else if (!IS_UNDEFINED(current)) { ... }
else { ... }.  There's no need for such a comment if you make the code clear and
explicit instead.  (It's especially surprising because it differs from all the
surrounding code, forcing the reader to think about it).

We will also generate slightly better code for IS_UNDEFINED(current) than using
conversion to boolean to test if it's undefined, so there's no reason to be
cryptic.

[Lasse Reichstein, 2010-12-10 11:21:45]

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js#newcode618
src/v8natives.js:618: var value =
Changed to:
 var value;  // Default value is undefined.
    if (desc.hasValue()) {
      value = desc.getValue();
    } else if (!IS_UNDEFINED(current)) {
      value = current.getValue();
    }

http://codereview.chromium.org/5773002/diff/1/test/mjsunit/object-define-prop...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/5773002/diff/1/test/mjsunit/object-define-prop...
test/mjsunit/object-define-property.js:880: 
More tests added, for both non-existing property, and for inherited value and
accessor properties,

[Kevin Millikin (Chromium), 2010-12-10 11:28:20]

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js#newcode618
src/v8natives.js:618: var value =
I still think that's too cute.  This is a long function.  If someone writes "var
value = 0" somewhere above won't this break?

[Lasse Reichstein, 2010-12-10 11:37:51]

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/5773002/diff/1/src/v8natives.js#newcode618
src/v8natives.js:618: var value =
True, good point.
I'll make a new CL and make it 
 var value = void 0;