Add GetPropertyAttribute method for Object in the API

Add GetPropertyAttribute method for Object in the API

Patch by Peter Varga.

BUG=none
TEST=cctest/test-api/PropertyAttributes

Committed: http://code.google.com/p/v8/source/detail?r=8679

[Mads Ager (chromium), 2011-07-12 07:51:30]

http://codereview.chromium.org/7321006/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7321006/diff/1/include/v8.h#newcode1439
include/v8.h:1439: * Gets the property attributes of a property.
You should document that this returns None if the property is not present and
otherwise returns the property attributes of the existing property.

http://codereview.chromium.org/7321006/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7321006/diff/1/src/api.cc#newcode2723
src/api.cc:2723: i::Handle<i::String> name =
i::Handle<i::String>::cast(key_obj);
This will not work if you pass in some other object than a string (such as a
number or object). You should handle this in the same way that Object::Get is
handled. You need to convert the key to a string if it isn't already.

http://codereview.chromium.org/7321006/diff/1/src/api.cc#newcode2725
src/api.cc:2725: has_pending_exception = (result == ABSENT);
Let's not misuse has_pending_exception for this. Just have the
EXCEPTION_BAILOUT_CHECK here. Then after that return
static_cast<PropertyAttribute>(NONE) if result is ABSENT.

[Peter Varga, 2011-07-12 09:31:21]

On 2011/07/12 07:51:30, Mads Ager wrote:
> http://codereview.chromium.org/7321006/diff/1/src/api.cc#newcode2725
> src/api.cc:2725: has_pending_exception = (result == ABSENT);
> Let's not misuse has_pending_exception for this. Just have the
> EXCEPTION_BAILOUT_CHECK here. Then after that return
> static_cast<PropertyAttribute>(NONE) if result is ABSENT.

Sorry, I don't really understand this part of the review.
I notice that I messed up the usage of has_pending_exception but I'm not sure
how the proper usage should look like.
Shouldn't I throw any exception when the property is absent?
Do you mean that this kind of checking:
  if (result == ABSENT) return NONE;
is enough instead of exception handling?

[Mads Ager (chromium), 2011-07-12 10:53:04]

On 2011/07/12 09:31:21, Peter Varga wrote:
> On 2011/07/12 07:51:30, Mads Ager wrote:
> > http://codereview.chromium.org/7321006/diff/1/src/api.cc#newcode2725
> > src/api.cc:2725: has_pending_exception = (result == ABSENT);
> > Let's not misuse has_pending_exception for this. Just have the
> > EXCEPTION_BAILOUT_CHECK here. Then after that return
> > static_cast<PropertyAttribute>(NONE) if result is ABSENT.
> 
> Sorry, I don't really understand this part of the review.
> I notice that I messed up the usage of has_pending_exception but I'm not sure
> how the proper usage should look like.
> Shouldn't I throw any exception when the property is absent?
> Do you mean that this kind of checking:
>   if (result == ABSENT) return NONE;
> is enough instead of exception handling?

If you want to throw an exception when the property is absent you should do so
explicitly. The EXCEPTION_BAILOUT_CHECK is used to propagate exceptions that
have already been scheduled. No exception is scheduled when attempting to get
the property attributes for a property. You will just be told that it is not
there. If you want to throw an exception in that case you should do so
explicitly. This indicates to me that there are tests missing too. The expected
behavior should have tests for this case. :-)

[Mads Ager (chromium), 2011-07-15 07:25:50]

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#newc...
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);

[Peter Varga, 2011-07-18 09:33:30]

Finally I have removed the exception throwing when the property doesn't exist.

[Mads Ager (chromium), 2011-07-18 17:34:56]

OK, LGTM

We have come full circle on this. There is no need for the changes in
handles.[h,cc] and objects.[h,cc]. I'll move all of the implementation to api.cc
and land for you.

Thanks for the patch.