Refactor prototype setting code and expose SetPrototype to public V8 API.

Refactor prototype setting code and expose SetPrototype to public V8 API.

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

[antonm, 2010-02-09 20:37:13]

Mads,

may you have a look?

[Mads Ager (chromium), 2010-02-10 08:00:46]

http://codereview.chromium.org/598020/diff/1/3
File src/accessors.cc (right):

http://codereview.chromium.org/598020/diff/1/3#newcode651
src/accessors.cc:651: Handle<Object> handle(value);
Add a HandleScope to this method to make sure this handle does not leak to an
outer handle scope.

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

http://codereview.chromium.org/598020/diff/1/4#newcode2041
src/api.cc:2041: return !self->SetPrototype(value_obj,
skip_hidden_prototypes)->IsFailure();
The failure return is for exceptions right?  We should create a handle based
version of SetPrototype in handles.cc which calls the one which does not use
handles and returns an empty handle in case of exceptions.  Then we need to use
the exception handling macros to make sure that we correctly reschedule
exceptions.  Have a look at how that is handled in Set including SetProperty in
handles.cc.

[antonm, 2010-02-10 12:48:29]

Thanks a lot for comments, Mads.  May you have another look?

http://codereview.chromium.org/598020/diff/1/3
File src/accessors.cc (right):

http://codereview.chromium.org/598020/diff/1/3#newcode651
src/accessors.cc:651: Handle<Object> handle(value);
On 2010/02/10 08:00:46, Mads Ager wrote:
> Add a HandleScope to this method to make sure this handle does not leak to an
> outer handle scope.

Done.

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

http://codereview.chromium.org/598020/diff/1/4#newcode2041
src/api.cc:2041: return !self->SetPrototype(value_obj,
skip_hidden_prototypes)->IsFailure();
On 2010/02/10 08:00:46, Mads Ager wrote:
> The failure return is for exceptions right?  We should create a handle based
> version of SetPrototype in handles.cc which calls the one which does not use
> handles and returns an empty handle in case of exceptions.  Then we need to
use
> the exception handling macros to make sure that we correctly reschedule
> exceptions.  Have a look at how that is handled in Set including SetProperty
in
> handles.cc.

Done.

[Mads Ager (chromium), 2010-02-10 13:19:41]

Thanks Anton, I would like to make sure that the exception throwing part of
SetPrototype works correctly.  Other than that it looks good.

http://codereview.chromium.org/598020/diff/1007/1010
File src/api.cc (right):

http://codereview.chromium.org/598020/diff/1007/1010#newcode2040
src/api.cc:2040: return !i::SetPrototype(self, value_obj).is_null();
I think we need to use the EXCEPTION_PREAMPLE and EXCEPTION_BAILOUT_CHECK macros
here to make sure that we correctly clear exceptions?  We should probably add
some tests of exception handling for this API method.

http://codereview.chromium.org/598020/diff/1007/1015
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/598020/diff/1007/1015#newcode4846
test/cctest/test-api.cc:4846: THREADED_TEST(SetPrototype) {
Could you add tests that attempt to create a cycle in the prototype chain?  That
will throw an exception and we should make sure that we can catch it with an API
TryCatch handler and that we can execute more code after we do that (to check
that the exception flags are cleared correctly).

[antonm, 2010-02-10 14:17:48]

Thanks a lot for spotting this.  Hopefully, I addressed your concern.  The main
problem I faced is how to fail if there is pending exception.  I just added an
explicit check, but if there is some other method, I'd be glad to update the
test.

On 2010/02/10 13:19:41, Mads Ager wrote:
> Thanks Anton, I would like to make sure that the exception throwing part of
> SetPrototype works correctly.  Other than that it looks good.
> 
> http://codereview.chromium.org/598020/diff/1007/1010
> File src/api.cc (right):
> 
> http://codereview.chromium.org/598020/diff/1007/1010#newcode2040
> src/api.cc:2040: return !i::SetPrototype(self, value_obj).is_null();
> I think we need to use the EXCEPTION_PREAMPLE and EXCEPTION_BAILOUT_CHECK
macros
> here to make sure that we correctly clear exceptions?  We should probably add
> some tests of exception handling for this API method.
> 
> http://codereview.chromium.org/598020/diff/1007/1015
> File test/cctest/test-api.cc (right):
> 
> http://codereview.chromium.org/598020/diff/1007/1015#newcode4846
> test/cctest/test-api.cc:4846: THREADED_TEST(SetPrototype) {
> Could you add tests that attempt to create a cycle in the prototype chain? 
That
> will throw an exception and we should make sure that we can catch it with an
API
> TryCatch handler and that we can execute more code after we do that (to check
> that the exception flags are cleared correctly).

[Mads Ager (chromium), 2010-02-10 14:31:59]

LGTM!