first step to remove unsafe handles

first step to remove unsafe handles

R=svenpanne@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14530

[Sven Panne, 2013-04-30 07:31:22]

First round of comments.

https://codereview.chromium.org/12729023/diff/10001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/12729023/diff/10001/include/v8.h#newcode557
include/v8.h:557: V8_INLINE(void Dispose());
Why is V8_DEPRECATED changed to V8_INLINE here and at a few other places? To me
it seems like a typo...

https://codereview.chromium.org/12729023/diff/10001/samples/shell.cc
File samples/shell.cc (right):

https://codereview.chromium.org/12729023/diff/10001/samples/shell.cc#newcode117
samples/shell.cc:117: return v8::Context::New(v8::Isolate::GetCurrent(), NULL,
global);
While we're here, can we make this an argument? The caller already has the
Isolate.

https://codereview.chromium.org/12729023/diff/10001/src/api.h
File src/api.h (right):

https://codereview.chromium.org/12729023/diff/10001/src/api.h#newcode262
src/api.h:262: class InternalHandleHelper {
Why do we need to stuff this template into a helper class? Even templates can
have friends... ;-)

https://codereview.chromium.org/12729023/diff/10001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/12729023/diff/10001/src/d8.cc#newcode1199
src/d8.cc:1199: Handle<Object> Shell::DebugMessageDetails(Handle<String>
message) {
Again, it might make sense to pass the Isolate as a parameter here and below.

https://codereview.chromium.org/12729023/diff/10001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/12729023/diff/10001/test/cctest/cctest.h#newc...
test/cctest/cctest.h:98: return
*reinterpret_cast<v8::Handle<v8::Context>*>(&context_);
Hmmm, this looks like an entry for the obfuscated C++ contest ;-), can we do
this differently? To be honest, I don't have an idea...

https://codereview.chromium.org/12729023/diff/10001/test/cctest/test-profile-...
File test/cctest/test-profile-generator.cc (right):

https://codereview.chromium.org/12729023/diff/10001/test/cctest/test-profile-...
test/cctest/test-profile-generator.cc:882: (*env)->Enter();
I am a bit confused: Why is this change necessary? Shouldn't Persistent have the
obvious operator*/operator-> pair?

[dcarney, 2013-04-30 07:43:21]

https://chromiumcodereview.appspot.com/12729023/diff/10001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/12729023/diff/10001/include/v8.h#newco...
include/v8.h:557: V8_INLINE(void Dispose());
On 2013/04/30 07:31:22, Sven Panne wrote:
> Why is V8_DEPRECATED changed to V8_INLINE here and at a few other places? To
me
> it seems like a typo...

we need to revert that behaviour and change handle functions not to have isolate
parameters.  we talked about this a bit earlier (a month ago, though...)

https://chromiumcodereview.appspot.com/12729023/diff/10001/src/api.h
File src/api.h (right):

https://chromiumcodereview.appspot.com/12729023/diff/10001/src/api.h#newcode262
src/api.h:262: class InternalHandleHelper {
On 2013/04/30 07:31:22, Sven Panne wrote:
> Why do we need to stuff this template into a helper class? Even templates can
> have friends... ;-)

I made this helper class a friend of Persistent and Handle

https://chromiumcodereview.appspot.com/12729023/diff/10001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://chromiumcodereview.appspot.com/12729023/diff/10001/test/cctest/cctest...
test/cctest/cctest.h:98: return
*reinterpret_cast<v8::Handle<v8::Context>*>(&context_);
On 2013/04/30 07:31:22, Sven Panne wrote:
> Hmmm, this looks like an entry for the obfuscated C++ contest ;-), can we do
> this differently? To be honest, I don't have an idea...

we've just created the fantastic class UnsafePersistent
https://chromiumcodereview.appspot.com/14443007/

the reinterprets are done in there. Want something like this here?

https://chromiumcodereview.appspot.com/12729023/diff/10001/test/cctest/test-p...
File test/cctest/test-profile-generator.cc (right):

https://chromiumcodereview.appspot.com/12729023/diff/10001/test/cctest/test-p...
test/cctest/test-profile-generator.cc:882: (*env)->Enter();
On 2013/04/30 07:31:22, Sven Panne wrote:
> I am a bit confused: Why is this change necessary? Shouldn't Persistent have
the
> obvious operator*/operator-> pair?


* and -> must die!

[Sven Panne, 2013-04-30 12:06:00]

lgtm

[dcarney, 2013-05-02 20:19:26]

Committed patchset #6 manually as r14530 (presubmit successful).