Overall, the change looks good. I think we have to be careful that we
do not search the prototype during propertyIsEnumerable. The ECMA-262
standard explictly states that propertyIsEnumerable does not consider
the prototype chain. Therefore, you would get:
var p = { x: 42 };
var o = { };
o.__proto__ = p;
propertyIsEnumerable(o.x); // false
for (var name in o) print(name); // prints x
I think all we have to do is propagate the continue_search parameter to
the GetAttributeWithFailedAccessCheck function and only search the
prototype in case it is true.
http://codereview.chromium.org/8834/diff/1/3
File src/objects.cc (right):
http://codereview.chromium.org/8834/diff/1/3#newcode270
Line 270: Object* receiver,
Parameters should be indented by four spaces instead of two.
http://codereview.chromium.org/8834/diff/1/3#newcode288
Line 288: case CONSTANT_FUNCTION: {
I think you might have to pass in a boolean indicating whether or not to
search the prototype chain. The ECMA-262 standard says that
propertyIsEnumerable does not take the prototype chain into account
(note to section 15.2.4.7), so we probably should not search for
accessors in the prototype chain in that case.
http://codereview.chromium.org/8834/diff/1/3#newcode301
Line 301: result->holder()->LookupRealNamedProperty(name, &r);
Similarly here, we probably need the boolean indicating whether or not
to search the prototype chain. That is, whether this should be a
LookupRealNamedProperty or only a LocalLookupRealNamedProperty.
http://codereview.chromium.org/8834/diff/1/3#newcode1772
Line 1772: return GetPropertyAttributeWithFailedAccessCheck(receiver,
result, name);
I think we need to propagate the continue_search argument here.
http://codereview.chromium.org/8834/diff/1/5
File src/objects.h (right):
http://codereview.chromium.org/8834/diff/1/5#newcode1452
Line 1452: Object* receiver,
This should be a four space indent. It looks like only three?
http://codereview.chromium.org/8834/diff/1/2
File test/cctest/test-api.cc (right):
http://codereview.chromium.org/8834/diff/1/2#newcode3427
Line 3427: " if (p == 'blocked_prop') return false;"
We should also check that accessible_prop is actually enumerated.
Is there a reason for not just doing "for (var p in other)" instead of
putting other in the prototype chain?
Also, I think using a couple of more lines and some more whitespace
would make the code evaluated more readable.
http://codereview.chromium.org/8834
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
