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