Added Object.defineProperty + needed internal functionality:...

Added Object.defineProperty + needed internal functionality:
  DefineOwnProperty (changed to allow for redefinition of existing property)
  SameValue
  Extra info on propertydescriptor
  GetProperty
  HasProperty

Currently the DefineOrRedefineAccessorProperty deletes the existing
property on the object if it is a dataproperty (FIELD or NORMAL) and
adds a new one. This can potentially be optimized.



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

[Mads Ager (chromium), 2010-01-29 08:56:24]

We should add API tests for this as well to see how we handle accessor
properties added through the API.  There are three cases for accessors and we
need to make sure that we can handle all three types.

http://codereview.chromium.org/555149/diff/1/8
File src/messages.js (right):

http://codereview.chromium.org/555149/diff/1/8#newcode165
src/messages.js:165: redefine_disallowed:          "Cannot redefine property :
%0",
Remove space before ':' in both of these message to match the other error
messages.

http://codereview.chromium.org/555149/diff/1/6
File src/runtime.cc (left):

http://codereview.chromium.org/555149/diff/1/6#oldcode576
src/runtime.cc:576: 
Accidental edit?  Please add this line back.

http://codereview.chromium.org/555149/diff/1/6
File src/runtime.cc (right):

http://codereview.chromium.org/555149/diff/1/6#newcode2931
src/runtime.cc:2931: // attributes.
I think this comment should be extended to make it clear what is going on.  For
simplicity you normalize the properties so you do not have to worry about
copying instance descriptors and changing the map of the object.  Also, as you
explained to me offline, a normal set property call does not currently change
the attributes of an existing property.

http://codereview.chromium.org/555149/diff/1/6#newcode2932
src/runtime.cc:2932: if (result.IsValid() && attr != result.GetAttributes()) {
Should IsValid be IsProperty here?

http://codereview.chromium.org/555149/diff/1/6#newcode2939
src/runtime.cc:2939: return Runtime::ForceSetObjectProperty(js_object, name,
obj_value, attr);
Do you really want to force the set here?  Isn't a normal set property enough?

http://codereview.chromium.org/555149/diff/1/5
File src/runtime.js (right):

http://codereview.chromium.org/555149/diff/1/5#newcode509
src/runtime.js:509: // ES5, section 9.2
ECMA-262, section 9.2, page 30.

To match the other cases.

http://codereview.chromium.org/555149/diff/1/5#newcode576
src/runtime.js:576: // Number
I would remove this comment.

http://codereview.chromium.org/555149/diff/1/5#newcode586
src/runtime.js:586: //String
I would remove this comment.

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

http://codereview.chromium.org/555149/diff/1/4#newcode511
src/v8natives.js:511: if (!IS_UNDEFINED(prop))
I would write this on one line of use '{' and '}' around the body.  Similarly
for the other one-line body ifs.

http://codereview.chromium.org/555149/diff/1/4#newcode532
src/v8natives.js:532: // Error handling according to spec
End comment with period.

http://codereview.chromium.org/555149/diff/1/4#newcode568
src/v8natives.js:568: if (desc.hasEnumerable())
Please use '{' and '}' around the bodies here.

http://codereview.chromium.org/555149/diff/1/2
File test/es5conform/es5conform.status (right):

http://codereview.chromium.org/555149/diff/1/2#newcode217
test/es5conform/es5conform.status:217: chapter15/15.2/15.2.3/15.2.3.6: PASS
Do you need lines for tests that pass?  Can't you just remove these lines.

http://codereview.chromium.org/555149/diff/1/2#newcode223
test/es5conform/es5conform.status:223: chapter15/15.2/15.2.3/15.2.3.14: PASS
Here is one as well, just delete the lines - the default is PASS?  There are
more lines like these and we should remove them all.

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

http://codereview.chromium.org/555149/diff/1/3#newcode63
test/mjsunit/object-define-property.js:63: var empDesc = {};
Spell out empty?

http://codereview.chromium.org/555149/diff/1/3#newcode65
test/mjsunit/object-define-property.js:65: var accConfigurable = {
Spell out accessor?

[Rico, 2010-02-01 13:27:39]

Added tests to test-api.cc for API defined property
Added extra test to object-define-property.js to test redefinition of accessors
defined using __defineSetter__ and __defineGetter__.
Changed getOwnProperty to also take into account accessors defined through the
API.

http://codereview.chromium.org/555149/diff/1/8
File src/messages.js (right):

http://codereview.chromium.org/555149/diff/1/8#newcode165
src/messages.js:165: redefine_disallowed:          "Cannot redefine property :
%0",
On 2010/01/29 08:56:24, Mads Ager wrote:
> Remove space before ':' in both of these message to match the other error
> messages.

Done.

http://codereview.chromium.org/555149/diff/1/6
File src/runtime.cc (left):

http://codereview.chromium.org/555149/diff/1/6#oldcode576
src/runtime.cc:576: 
On 2010/01/29 08:56:24, Mads Ager wrote:
> Accidental edit?  Please add this line back.

Done.

http://codereview.chromium.org/555149/diff/1/6
File src/runtime.cc (right):

http://codereview.chromium.org/555149/diff/1/6#newcode2931
src/runtime.cc:2931: // attributes.
On 2010/01/29 08:56:24, Mads Ager wrote:
> I think this comment should be extended to make it clear what is going on. 
For
> simplicity you normalize the properties so you do not have to worry about
> copying instance descriptors and changing the map of the object.  Also, as you
> explained to me offline, a normal set property call does not currently change
> the attributes of an existing property.

Done.

http://codereview.chromium.org/555149/diff/1/6#newcode2932
src/runtime.cc:2932: if (result.IsValid() && attr != result.GetAttributes()) {
On 2010/01/29 08:56:24, Mads Ager wrote:
> Should IsValid be IsProperty here?

Done.

http://codereview.chromium.org/555149/diff/1/6#newcode2939
src/runtime.cc:2939: return Runtime::ForceSetObjectProperty(js_object, name,
obj_value, attr);
On 2010/01/29 08:56:24, Mads Ager wrote:
> Do you really want to force the set here?  Isn't a normal set property enough?

Done.

http://codereview.chromium.org/555149/diff/1/5
File src/runtime.js (right):

http://codereview.chromium.org/555149/diff/1/5#newcode509
src/runtime.js:509: // ES5, section 9.2
On 2010/01/29 08:56:24, Mads Ager wrote:
> ECMA-262, section 9.2, page 30.
> 
> To match the other cases.

Done.

http://codereview.chromium.org/555149/diff/1/5#newcode576
src/runtime.js:576: // Number
On 2010/01/29 08:56:24, Mads Ager wrote:
> I would remove this comment.

Done.

http://codereview.chromium.org/555149/diff/1/5#newcode586
src/runtime.js:586: //String
On 2010/01/29 08:56:24, Mads Ager wrote:
> I would remove this comment.

Done.

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

http://codereview.chromium.org/555149/diff/1/4#newcode511
src/v8natives.js:511: if (!IS_UNDEFINED(prop))
On 2010/01/29 08:56:24, Mads Ager wrote:
> I would write this on one line of use '{' and '}' around the body.  Similarly
> for the other one-line body ifs.

Done.

http://codereview.chromium.org/555149/diff/1/4#newcode532
src/v8natives.js:532: // Error handling according to spec
On 2010/01/29 08:56:24, Mads Ager wrote:
> End comment with period.

Done.

http://codereview.chromium.org/555149/diff/1/4#newcode568
src/v8natives.js:568: if (desc.hasEnumerable())
On 2010/01/29 08:56:24, Mads Ager wrote:
> Please use '{' and '}' around the bodies here.

Done.

http://codereview.chromium.org/555149/diff/1/2
File test/es5conform/es5conform.status (right):

http://codereview.chromium.org/555149/diff/1/2#newcode217
test/es5conform/es5conform.status:217: chapter15/15.2/15.2.3/15.2.3.6: PASS
On 2010/01/29 08:56:24, Mads Ager wrote:
> Do you need lines for tests that pass?  Can't you just remove these lines.
Right - removed these and all the old ones

http://codereview.chromium.org/555149/diff/1/2#newcode223
test/es5conform/es5conform.status:223: chapter15/15.2/15.2.3/15.2.3.14: PASS
On 2010/01/29 08:56:24, Mads Ager wrote:
> Here is one as well, just delete the lines - the default is PASS?  There are
> more lines like these and we should remove them all.

Done.

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

http://codereview.chromium.org/555149/diff/1/3#newcode63
test/mjsunit/object-define-property.js:63: var empDesc = {};
On 2010/01/29 08:56:24, Mads Ager wrote:
> Spell out empty?

Done.

http://codereview.chromium.org/555149/diff/1/3#newcode65
test/mjsunit/object-define-property.js:65: var accConfigurable = {
On 2010/01/29 08:56:24, Mads Ager wrote:
> Spell out accessor?

Done.

[Mads Ager (chromium), 2010-02-01 17:36:14]

This is definitely getting there.

Maybe we should have some tests in test-api that at least documents how this
interact with interceptors as well?

http://codereview.chromium.org/555149/diff/5002/6005
File src/runtime.cc (right):

http://codereview.chromium.org/555149/diff/5002/6005#newcode601
src/runtime.cc:601: // and API defined callback.
and -> an

http://codereview.chromium.org/555149/diff/5002/6005#newcode2907
src/runtime.cc:2907: if (result.type() == FIELD || result.type() == NORMAL )
How about CONSTANT_FUNCTION?

Is CALLBACK handled correctly if there already is a CALLBACK property?

http://codereview.chromium.org/555149/diff/5002/6005#newcode2944
src/runtime.cc:2944: return Runtime::ForceSetObjectProperty(js_object, name,
obj_value, attr);
Did you need the ForceSet here?

http://codereview.chromium.org/555149/diff/5002/6008
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/555149/diff/5002/6008#newcode2308
test/cctest/test-api.cc:2308: Local<Script> script_desc =
Script::Compile(v8_str("var prop ="
You could give yourself a bit more horizontal space by doing:

Local<Script> script_desc
    = Script::Compile(v8_str("..."
                             "..."));

http://codereview.chromium.org/555149/diff/5002/6008#newcode2316
test/cctest/test-api.cc:2316: Local<Script> script_define =
Move '=' to next line and do a four-space indent.  Repeated below.

http://codereview.chromium.org/555149/diff/5002/6008#newcode2317
test/cctest/test-api.cc:2317: Script::Compile(v8_str("var desc = {get:
function(){return 42; },"
Space before "get:".  Repeated below.

[Rico, 2010-02-02 17:07:07]

Added test for CONSTANT_FUNCTION and extra tests for CALLBACKS in case these are
partially redefined (i.e., we only change the get or set).

http://codereview.chromium.org/555149/diff/5002/6005
File src/runtime.cc (right):

http://codereview.chromium.org/555149/diff/5002/6005#newcode601
src/runtime.cc:601: // and API defined callback.
On 2010/02/01 17:36:14, Mads Ager wrote:
> and -> an

Done.

http://codereview.chromium.org/555149/diff/5002/6005#newcode2907
src/runtime.cc:2907: if (result.type() == FIELD || result.type() == NORMAL )
On 2010/02/01 17:36:14, Mads Ager wrote:
> How about CONSTANT_FUNCTION?

I have added a check to object-define-property.js where I test for
CONSTANT_FUNCTION - this works fine without deleting it first.
> 
> Is CALLBACK handled correctly if there already is a CALLBACK property?
As discussed offline I have added some new test to check that the it actually
behaves correctly if there is already a CALLBACK property (with both or just one
of {get, set}) and we do a redefine with a descriptor that has only a get or
set. This works as expected (i.e., the get or set that is undefined in the
descriptor keeps it's existing value). The reason is that the DefineAccessor
only redefines the getter or setter field (this method is actually called twice
if we call defineProperty with a descriptor that has both a getter and a setter,
once for each).

http://codereview.chromium.org/555149/diff/5002/6005#newcode2944
src/runtime.cc:2944: return Runtime::ForceSetObjectProperty(js_object, name,
obj_value, attr);
On 2010/02/01 17:36:14, Mads Ager wrote:
> Did you need the ForceSet here?

Done.

http://codereview.chromium.org/555149/diff/5002/6008
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/555149/diff/5002/6008#newcode2308
test/cctest/test-api.cc:2308: Local<Script> script_desc =
Script::Compile(v8_str("var prop ="
On 2010/02/01 17:36:14, Mads Ager wrote:
> You could give yourself a bit more horizontal space by doing:
> 
> Local<Script> script_desc
>     = Script::Compile(v8_str("..."
>                              "..."));
>                              

Done.

http://codereview.chromium.org/555149/diff/5002/6008#newcode2316
test/cctest/test-api.cc:2316: Local<Script> script_define =
On 2010/02/01 17:36:14, Mads Ager wrote:
> Move '=' to next line and do a four-space indent.  Repeated below.

Done.

http://codereview.chromium.org/555149/diff/5002/6008#newcode2317
test/cctest/test-api.cc:2317: Script::Compile(v8_str("var desc = {get:
function(){return 42; },"
On 2010/02/01 17:36:14, Mads Ager wrote:
> Space before "get:".  Repeated below.

Done.

[Rico, 2010-02-03 12:02:30]

Changed DefineOrRedefineAccessor to also delete CONSTANT_FUNCTION properties
before redefining it as an accessor (in case we have internal functions that are
readonly but can be overridden - users cannot define readonly CONSTANT_FUNCTION
properties using Object.defineProperty)

[Mads Ager (chromium), 2010-02-03 12:17:50]

LGTM