[api] expose API for adding per-context Intrinsics to Templates

[api] expose API for adding per-context Intrinsics to Templates

Add mechanism for adding per-context ECMAScript intrinsics to Template
objects, which may be instantiated in multiple contexts.

BUG=
LOG=N
R=jochen@chromium.org, verwaest@chromium.org

Committed: https://crrev.com/1be2b46c854af7314aae0111ff70ee8a5766135d
Cr-Commit-Position: refs/heads/master@{#31450}

[caitp (gmail), 2015-10-14 23:01:53]

PTAL, trying to add a way to install per-context builtins on Template objects
which can be instantiated on multiple contexts.

It's kind of terrible in a way, but I'm not sure how else to do it.

Related to https://codereview.chromium.org/1381413003/

[caitp (gmail), 2015-10-19 13:32:35]

Description was changed from

==========
[api] expose API for adding per-context Intrinsics to Templates

Add mechanism for adding per-context ECMAScript intrinsics to Template
objects, which may be instantiated in multiple contexts.

BUG=
LOG=N
R=jochen@chromium.org, verwaest@chromium.org
==========

to

==========
[api] expose API for adding per-context Intrinsics to Templates

Add mechanism for adding per-context ECMAScript intrinsics to Template
objects, which may be instantiated in multiple contexts.

BUG=
LOG=N
R=jochen@chromium.org, verwaest@chromium.org
==========

[caitp (gmail), 2015-10-19 13:32:35]

caitpotter88@gmail.com changed reviewers:
+ adamk@chromium.org

[caitp (gmail), 2015-10-19 13:33:51]

On 2015/10/14 23:01:53, caitp wrote:
> PTAL, trying to add a way to install per-context builtins on Template objects
> which can be instantiated on multiple contexts.
> 
> It's kind of terrible in a way, but I'm not sure how else to do it.
> 
> Related to https://codereview.chromium.org/1381413003/

+adam, but it would be super helpful to get a look from API owners and Toon who
mentioned the security implications of the previous attempt

[jochen (gone - plz use gerrit), 2015-10-20 09:37:12]

I defer to Toon

[Toon Verwaest, 2015-10-20 15:00:54]

I'm not really a fan of this CL, but at the same time I don't know of a better
way to do it... It seems pretty nicely encapsulated and at least is pretty
declarative.

From discussing with adamk, it sounds like we'll need to expose at least a few
such "intrinsics"? (I don't really like the name Intrinsics that much...)

https://codereview.chromium.org/1409593002/diff/1/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1409593002/diff/1/src/api-natives.cc#newcode163
src/api-natives.cc:163: Context* GetCurrentContext(Isolate* isolate) {
This seems unnecessary given that context->iname() above will crash anyway. What
about just using isolate->native_context() in GetIntrinsic, passing the isolate
in there?

https://codereview.chromium.org/1409593002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1409593002/diff/1/src/api.cc#newcode1361
src/api.cc:1361: // TODO(dcarney): split api to allow values of v8::Value or
v8::TemplateInfo.
dcarney doesn't work on v8 anymore

[caitp (gmail), 2015-10-20 15:38:06]

Im not a big fan of the implementation, but at least it works. I'm not sure how
many other internal methods need to be exposed to the api.

About the name, it matches the spec name for these concepts, but if something
else is better, we can do that.

https://codereview.chromium.org/1409593002/diff/1/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1409593002/diff/1/src/api-natives.cc#newcode163
src/api-natives.cc:163: Context* GetCurrentContext(Isolate* isolate) {
On 2015/10/20 15:00:54, Toon Verwaest wrote:
> This seems unnecessary given that context->iname() above will crash anyway.
What
> about just using isolate->native_context() in GetIntrinsic, passing the
isolate
> in there?

Ok

https://codereview.chromium.org/1409593002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1409593002/diff/1/src/api.cc#newcode1361
src/api.cc:1361: // TODO(dcarney): split api to allow values of v8::Value or
v8::TemplateInfo.
On 2015/10/20 15:00:54, Toon Verwaest wrote:
> dcarney doesn't work on v8 anymore

Copy/pasted from one of the similar methods. Do you know if it's still valid?
Afaik the api allows this already right?

[Toon Verwaest, 2015-10-21 16:02:48]

lgtm

https://codereview.chromium.org/1409593002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1409593002/diff/1/src/api.cc#newcode1361
src/api.cc:1361: // TODO(dcarney): split api to allow values of v8::Value or
v8::TemplateInfo.
On 2015/10/20 15:38:06, caitp wrote:
> On 2015/10/20 15:00:54, Toon Verwaest wrote:
> > dcarney doesn't work on v8 anymore
> 
> Copy/pasted from one of the similar methods. Do you know if it's still valid?
> Afaik the api allows this already right?

What it probably wants is what jochen is working on now: support only Primitive
and Template. That would've prevented the scary bug you accidentally introduced
in your previous CL.

[caitp (gmail), 2015-10-21 19:33:49]

On 2015/10/21 16:02:48, Toon Verwaest wrote:
> lgtm
> 
> https://codereview.chromium.org/1409593002/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/1409593002/diff/1/src/api.cc#newcode1361
> src/api.cc:1361: // TODO(dcarney): split api to allow values of v8::Value or
> v8::TemplateInfo.
> On 2015/10/20 15:38:06, caitp wrote:
> > On 2015/10/20 15:00:54, Toon Verwaest wrote:
> > > dcarney doesn't work on v8 anymore
> > 
> > Copy/pasted from one of the similar methods. Do you know if it's still
valid?
> > Afaik the api allows this already right?
> 
> What it probably wants is what jochen is working on now: support only
Primitive
> and Template. That would've prevented the scary bug you accidentally
introduced
> in your previous CL.

Ah. But primitives already work, aren't per-context, can't have properties, etc.
Anyays, this should be good for now. If a better way to do it comes along, can
replace this with it

[caitp (gmail), 2015-10-21 19:40:21]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-10-21 19:40:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409593002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409593002/40001

[commit-bot: I haz the power, 2015-10-21 19:42:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-21 19:42:53]

Try jobs failed on following builders:
  v8_linux64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/10925)
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/10813)

[caitp (gmail), 2015-10-21 19:50:10]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-21 19:50:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1409593002/#ps60001
(title: "Fix bot failure")

[commit-bot: I haz the power, 2015-10-21 19:50:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409593002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409593002/60001

[commit-bot: I haz the power, 2015-10-21 20:14:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-21 20:14:59]

Patchset 4 (id:??) landed as
https://crrev.com/1be2b46c854af7314aae0111ff70ee8a5766135d
Cr-Commit-Position: refs/heads/master@{#31450}