Fixed, except:
'other' is in the prototype chain because you cannot actually enumerate the
properties of an object that you don't have access to. For the same reason
'accessible_prop' isn't actually enumerated. The test have been modified to
test for that as well.
On Tue, Oct 28, 2008 at 9:02 PM, <[EMAIL PROTECTED]> wrote:
> 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
>
--
--
Ole I Hougaard
Google Aarhus, Denmark
+45 8745 9215
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
