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?
http://codereview.chromium.org/555149
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
