Peter, I think the change itself is getting close.
I have a couple of questions below. I would prefer to not throw a
ReferenceError
here. At least if we do throw a reference error I would like the message
text to
be different from a normal ReferenceError (something like "cannot get
property
attributes for non-existing property 'x'").
What do you think?
http://codereview.chromium.org/7321006/diff/7001/include/v8.h
File include/v8.h (right):
http://codereview.chromium.org/7321006/diff/7001/include/v8.h#newcode1442
include/v8.h:1442: * Note: Throws a ReferenceError exception and returns
None when the property
I'm not sure I like the idea of throwing a ReferenceError exception
here. Normally reference errors are only thrown when loading a
non-existing global property. Throwing one here because someone
attempted to get property attributes of some non-existing property will
be confusing.
I think I would prefer to never throw an exception here. The user would
then have to either know that the property is there or use Has before
using GetPropertyAttributes.
http://codereview.chromium.org/7321006/diff/7001/include/v8.h#newcode1445
include/v8.h:1445: V8EXPORT PropertyAttribute
GetPropertyAttribute(Handle<Value> key);
GetPropertyAttribute -> GetPropertyAttributes
http://codereview.chromium.org/7321006/diff/7001/src/api.cc
File src/api.cc (right):
http://codereview.chromium.org/7321006/diff/7001/src/api.cc#newcode2717
src/api.cc:2717: self->TryGetPropertyAttribute(key_obj,
&has_pending_exception);
Could you put this one in handles.[h,cc] as is done with GetProperty?
i::GetPropertyAttributes(self, key_obj, &has_pending_exception)
http://codereview.chromium.org/7321006/diff/7001/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/7321006/diff/7001/src/objects.cc#newcode2622
src/objects.cc:2622: // Throws an exception when the property doesn't
exist.
Let's just return NONE in that case and document that behavior? Then the
user can use a combination of Has and GetPropertyAttributes if he
doesn't know if the property exists?
http://codereview.chromium.org/7321006/diff/7001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
http://codereview.chromium.org/7321006/diff/7001/test/cctest/test-api.cc#newcode2088
test/cctest/test-api.cc:2088: try_catch.Reset();
You should add a test where the string conversion throws an exception.
Something like:
Local<Value> key = CompileRun("({ toString: function() { throw 42; }
})");
...->GetPropertyAttributes(key);
http://codereview.chromium.org/7321006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
