Implement CallAsConstructor method for Object in the API

Implement CallAsConstructor method for Object in the API

Patch by Peter Varga.

BUG=v8:1348
TEST=cctest/test-api/ConstructorForObject

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

[Mads Ager (chromium), 2011-04-28 12:14:42]

Similar comments as to the Call patch. Is this driven by an actual need for
this?

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

http://codereview.chromium.org/6902108/diff/1/include/v8.h#newcode1693
include/v8.h:1693: V8EXPORT Local<Object> NewInstance(int argc, Handle<Value>
argv[]) const;
NewInstance as a name makes sense for functions but I don't think it makes sense
for objects. Maybe name this something like CallAsConstructor.

Please add a comment explaining when it would make sense to call an object as a
constructor and why it makes sense.

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

http://codereview.chromium.org/6902108/diff/1/src/api.cc#newcode3214
src/api.cc:3214:
i::Handle<i::JSFunction>::cast(i::Execution::GetConstructorDelegate(obj));
Use four space indent here.

http://codereview.chromium.org/6902108/diff/1/src/api.cc#newcode3216
src/api.cc:3216: i::Execution::Call(delegate, obj, argc, args,
&has_pending_exception);
four space indent instead of two

http://codereview.chromium.org/6902108/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6902108/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:6757: if (proto->IsObject())
Use braces around if bodies or make this a one-liner.

http://codereview.chromium.org/6902108/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:6783: Local<Value> args[] = { v8_num(29) };
Please add more testing. Test calling on a random object which does not have a
call as function handler. Test exceptions thrown in the construct callback.

[Peter Varga, 2011-05-04 17:41:41]

Fix patch like in case of the Object::CallAsFunction patch.

The order of results is reverted in case of exception checking in CHECK_EQ
macro.

[Peter Varga, 2011-05-05 08:34:16]

Sorry, It seems this patch is also failing in debug mode. I need to do another
check...

[Mads Ager (chromium), 2011-05-05 09:03:33]

http://codereview.chromium.org/6902108/diff/5001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6902108/diff/5001/include/v8.h#newcode1623
include/v8.h:1623: Handle<Value> argv[]) const;
I would remove the const here for consistency with the other methods.

http://codereview.chromium.org/6902108/diff/5001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6902108/diff/5001/src/api.cc#newcode3297
src/api.cc:3297: HandleScope scope;
Use the isolate here.

http://codereview.chromium.org/6902108/diff/5001/src/api.cc#newcode3317
src/api.cc:3317: return
scope.Close(Utils::ToLocal(i::Handle<i::JSObject>::cast(returned)));
Let's remove the cast to JSObject here. The function delegate can return a
string or number which is not a JSObject?

http://codereview.chromium.org/6902108/diff/5001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6902108/diff/5001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:6752: if (args[0]->IsInt32()) {
Add a test where the input is a string, undefined, null, or something like that.
A basic value that is not an object?

[Peter Varga, 2011-05-05 09:54:03]

First of all thanks for reviews.
Here are some things which aren't clear for me:

http://codereview.chromium.org/6902108/diff/5001/src/api.cc#newcode3297
> src/api.cc:3297: HandleScope scope;
> Use the isolate here.

There are two different HandleScope in V8: i::HandleScope and v8::HandleScope

If I use the internal one then I can use isolate but I can't close the scope.
Does the i::HandleScope::CloseAndEscape and v8::HandleScope::Close method the
same thing?

If I use the v8::HandleScope then I can't use isolate.

Which is the preferable one in this case?
What is the purpose of the two different HandleScope?

http://codereview.chromium.org/6902108/diff/5001/src/api.cc#newcode3317
> src/api.cc:3317: return
> scope.Close(Utils::ToLocal(i::Handle<i::JSObject>::cast(returned)));
> Let's remove the cast to JSObject here. The function delegate can return a
> string or number which is not a JSObject?
Can Utils::OpenHandle(this) be a JSFunction?

If it can then the Object::CallAsConstructor should work same like the
Function::NewInstance which returns a v8::Object therefore the cast to JSObject
is needed.

In other case the Object::CallAsConstructor should returns v8::Value and the
cast isn't needed and the handling of (obj->IsJSFunction) case isn't necessary
(in the Object::CallAsFunction method as well).

[Mads Ager (chromium), 2011-05-05 10:25:00]

On 2011/05/05 09:54:03, Peter Varga wrote:
> First of all thanks for reviews.
> Here are some things which aren't clear for me:
> 
> http://codereview.chromium.org/6902108/diff/5001/src/api.cc#newcode3297
> > src/api.cc:3297: HandleScope scope;
> > Use the isolate here.
> 
> There are two different HandleScope in V8: i::HandleScope and v8::HandleScope
> 
> If I use the internal one then I can use isolate but I can't close the scope.
> Does the i::HandleScope::CloseAndEscape and v8::HandleScope::Close method the
> same thing?
> 
> If I use the v8::HandleScope then I can't use isolate.
> 
> Which is the preferable one in this case?
> What is the purpose of the two different HandleScope?

Yes, the internal CloseAndEscape is the same thing as Close. The handle scopes
are really the same under the surface. The external HandleScope is a simple
interface for users to create handle scopes through the API.

> http://codereview.chromium.org/6902108/diff/5001/src/api.cc#newcode3317
> > src/api.cc:3317: return
> > scope.Close(Utils::ToLocal(i::Handle<i::JSObject>::cast(returned)));
> > Let's remove the cast to JSObject here. The function delegate can return a
> > string or number which is not a JSObject?
> Can Utils::OpenHandle(this) be a JSFunction?
> 
> If it can then the Object::CallAsConstructor should work same like the
> Function::NewInstance which returns a v8::Object therefore the cast to
JSObject
> is needed.
> 
> In other case the Object::CallAsConstructor should returns v8::Value and the
> cast isn't needed and the handling of (obj->IsJSFunction) case isn't necessary
> (in the Object::CallAsFunction method as well).

Right, my question is really whether or not it is possible to get a non-object
out here. If it isn't you can keep the cast. In any case it would be good to add
a test case where you return a non-object from the call as function handler and
check that what is returned is an object.

[Mads Ager (chromium), 2011-05-06 10:41:47]

LGTM, I'll land.

http://codereview.chromium.org/6902108/diff/5008/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6902108/diff/5008/src/api.cc#newcode3300
src/api.cc:3300: i::Handle<i::Object> returned;
I would move these to where they are used. I'll take care of than when landing.

http://codereview.chromium.org/6902108/diff/5008/src/api.cc#newcode3308
src/api.cc:3308: i::Handle<i::JSObject>::cast(returned)));
I would format this differently. I'll take care of that when landing.