Let the embedder store arbitrary Values via Context::SetData

Let the embedder store arbitrary Values via Context::SetData

In WebKit, we would like to store a void* to a data structure that contains
lots of exciting per-context data. The current API restricts us to storing only
Strings, which is less useful.

I've also cleaned up the implementation of GetData to be less convoluted.

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

[abarth-chromium, 2012-09-12 00:54:42]

@vegorov: Feel free to bounce this to another reviewer.  I'm not sure who to
send patches to.

Thanks!

[abarth-chromium, 2012-09-12 01:05:13]

I'm told danno needs to review this path because it touches the API.

[Sven Panne, 2012-09-13 07:39:28]

Just a few drive-by comments...

A more general note: Our API is very inconsistent regarding the types of
associated data. With this change we now have Handle<String>, Handle<Value> and
void*. Perhaps we should change all these types to void*, but this would be a
more radical change.

https://chromiumcodereview.appspot.com/10907189/diff/1/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10907189/diff/1/include/v8.h#newcode3706
include/v8.h:3706: void SetData(Handle<Value> data);
I think that this change should be OK and not cause any trouble for other
embedders because Value is a superclass of String and there is no overloading
which could cause any trouble.

https://chromiumcodereview.appspot.com/10907189/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/10907189/diff/1/src/api.cc#newcode785
src/api.cc:785: if (!env->IsNativeContext()) {
I see that this has been here before, but it looks extremely obscure:

 * If we consider the fact that env is not a native context a bug caused by the
embedder, the we should report this via ApiCheck before returning an empty Value
and remove the ASSERT.

 * Otherwise we should remove the 'if' part and leave the ASSERT.

[abarth-chromium, 2012-09-13 07:42:59]

> A more general note: Our API is very inconsistent regarding the types of
> associated data. With this change we now have Handle<String>, Handle<Value>
and
> void*. Perhaps we should change all these types to void*, but this would be a
> more radical change.

I would prefer void* because that's what I intend to store there, but that would
necessitate either breaking API compatibility or introducing a new API.  I
tested this approach using External::Wrap and External::Unwrap and it was fast
enough from my use case.

> https://chromiumcodereview.appspot.com/10907189/diff/1/include/v8.h
> File include/v8.h (right):
> 
>
https://chromiumcodereview.appspot.com/10907189/diff/1/include/v8.h#newcode3706
> include/v8.h:3706: void SetData(Handle<Value> data);
> I think that this change should be OK and not cause any trouble for other
> embedders because Value is a superclass of String and there is no overloading
> which could cause any trouble.

Yeah, that was my hope.

> https://chromiumcodereview.appspot.com/10907189/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://chromiumcodereview.appspot.com/10907189/diff/1/src/api.cc#newcode785
> src/api.cc:785: if (!env->IsNativeContext()) {
> I see that this has been here before, but it looks extremely obscure:
> 
>  * If we consider the fact that env is not a native context a bug caused by
the
> embedder, the we should report this via ApiCheck before returning an empty
Value
> and remove the ASSERT.
> 
>  * Otherwise we should remove the 'if' part and leave the ASSERT.

I just was straightening out the existing logic.  This part of the CL can be
dropped if you'd prefer, but the original code was kind of convoluted and I
thought it would be useful to clean it up slightly.

[danno, 2012-09-14 13:15:13]

lgtm

LGTM

https://chromiumcodereview.appspot.com/10907189/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/10907189/diff/1/src/api.cc#newcode785
src/api.cc:785: if (!env->IsNativeContext()) {
It's probably outside of the scope of this CL to fix the weirdness that Sven
mentions here. 
On 2012/09/13 07:39:28, Sven Panne wrote:
> I see that this has been here before, but it looks extremely obscure:
> 
>  * If we consider the fact that env is not a native context a bug caused by
the
> embedder, the we should report this via ApiCheck before returning an empty
Value
> and remove the ASSERT.
> 
>  * Otherwise we should remove the 'if' part and leave the ASSERT.

[abarth-chromium, 2012-09-14 17:45:22]

> LGTM

Thanks!  I see that you've landed the patch in
<https://code.google.com/p/v8/source/detail?r=12520>.  Do you have a rough guess
for when this CL will be visible in Chromium?  I'm not in a bug rush, I'd like
like to set my expectations.  :)

> It's probably outside of the scope of this CL to fix the weirdness that Sven
> mentions here. 

I'll prepare a follow up CL.

Thanks again.