Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(959)

Issue 463040: Add Object.create from ECMAScript5. Supports value, writable, enumerable, ge... (Closed)

Created:
11 years ago by Erik Corry
Modified:
11 years ago
CC:
v8-dev
Visibility:
Public.

Description

Add Object.create from ECMAScript5. Supports value, writable, enumerable, get and set. Doesn't support configurable yet. See http://code.google.com/p/v8/issues/detail?id=460 Committed: http://code.google.com/p/v8/source/detail?r=3438

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 27

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+444 lines, -1 line) Patch
M src/messages.js View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M src/v8natives.js View 1 2 3 3 chunks +205 lines, -1 line 3 comments Download
A test/mjsunit/object-create.js View 1 2 3 4 1 chunk +234 lines, -0 lines 1 comment Download

Messages

Total messages: 7 (0 generated)
Erik Corry
11 years ago (2009-12-07 13:52:01 UTC) #1
Christian Plesner Hansen
Lots of comments. Needs a second iteration. http://codereview.chromium.org/463040/diff/5/1006 File src/messages.js (right): http://codereview.chromium.org/463040/diff/5/1006#newcode162 src/messages.js:162: value_and_getter_setter: "Invalid ...
11 years ago (2009-12-08 04:30:52 UTC) #2
Erik Corry
11 years ago (2009-12-08 10:19:40 UTC) #3
Erik Corry
New version uploaded. http://codereview.chromium.org/463040/diff/5/1006 File src/messages.js (right): http://codereview.chromium.org/463040/diff/5/1006#newcode162 src/messages.js:162: value_and_getter_setter: "Invalid property. 'value' present on ...
11 years ago (2009-12-08 10:20:00 UTC) #4
Lasse Reichstein
Drive by comments. http://codereview.chromium.org/463040/diff/5/1004 File test/mjsunit/object-create.js (right): http://codereview.chromium.org/463040/diff/5/1004#newcode43 test/mjsunit/object-create.js:43: var Foo = { foo: function() ...
11 years ago (2009-12-08 11:35:46 UTC) #5
Erik Corry
http://codereview.chromium.org/463040/diff/5/1004 File test/mjsunit/object-create.js (right): http://codereview.chromium.org/463040/diff/5/1004#newcode184 test/mjsunit/object-create.js:184: print("ok"); On 2009/12/08 11:35:46, Lasse Reichstein wrote: > Please ...
11 years ago (2009-12-08 13:52:23 UTC) #6
Christian Plesner Hansen
11 years ago (2009-12-08 20:44:40 UTC) #7
Lgtm, with more comments.

http://codereview.chromium.org/463040/diff/5/1006
File src/messages.js (right):

http://codereview.chromium.org/463040/diff/5/1006#newcode162
src/messages.js:162: value_and_getter_setter:      "Invalid property.  'value'
present on property with getter or setter",
I am very strongly against giving lower quality error messages to match JSC. 
It's up to you to decide what to do but you should at least consider giving a
better message and rebaselining the test.

http://codereview.chromium.org/463040/diff/5/1005
File src/v8natives.js (right):

http://codereview.chromium.org/463040/diff/5/1005#newcode312
src/v8natives.js:312: throw MakeTypeError(property_desc_object, [obj]);
*Ahem*

http://codereview.chromium.org/463040/diff/5/1005#newcode316
src/v8natives.js:316: var enumerable = obj.enumerable;
How is this not premature optimization?

http://codereview.chromium.org/463040/diff/5/1005#newcode360
src/v8natives.js:360: this.value_ = void 0;
> These property descriptors are never exposed to the user.  The spec does not
> give any details on how they should be implemented.  The
FromPropertyDescriptor
> method in 8.10.4 makes a new object with a well-defined structure which is
> passed to the user.

Okay, ignore my comment then.

http://codereview.chromium.org/463040/diff/9/11#newcode327
src/v8natives.js:327: if (value !== void 0 || value in obj)
desc.set_value(value);
IS_UNDEFINED

http://codereview.chromium.org/463040/diff/9/11#newcode344
src/v8natives.js:344: if (set !== void 0 && typeof(set) !== 'function') {
IS_UNDEFINED

http://codereview.chromium.org/463040/diff/9/11#newcode367
src/v8natives.js:367: this.get_set_ = false;
Calling a boolean xxxSet is not a particularly obvious choice and in this case,
where xxx is 'get' and 'set', it is really confusing.  Consider hasGetter,
hasSetter instead.

http://codereview.chromium.org/463040/diff/9/10
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/463040/diff/9/10#newcode43
test/mjsunit/object-create.js:43: var foo_proto = { foo: function() { ctr++; }};
lowerCamelCase.

Powered by Google App Engine
This is Rietveld 408576698