[api] Expose FunctionCallbackInfo::NewTarget

[api] Expose FunctionCallbackInfo::NewTarget

This is needed by Blink to implement the Custom Elements spec.

BUG=v8:4261
LOG=y

Committed: https://crrev.com/306c412ce04a7c21a208e33dfce530536453c20d
Cr-Commit-Position: refs/heads/master@{#35833}

[Benedikt Meurer, 2016-04-23 11:45:40]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-04-23 11:45:41]

Some early comments.

https://codereview.chromium.org/1910253005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1910253005/diff/1/include/v8.h#newcode3207
include/v8.h:3207: static const int kIsolateIndex = 1;
It'd be nice to remove isolate and callee here (not in the same CL, but
soonish). The isolate can be retrieved from the holder (which is always a
HeapObject), and the callee is deprecated for some time.

https://codereview.chromium.org/1910253005/diff/1/include/v8.h#newcode3222
include/v8.h:3222: int is_construct_call_;
This can now be removed (and potentially everything necessary to thread it
through), as you can tell whether you were invoked via [[Call]] or [[Construct]]
by checking if NewTarget is undefined. And IsConstructCall() above can do
exactly that.

[Benedikt Meurer, 2016-04-23 11:57:47]

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc#newcode4481
src/builtins.cc:4481: Isolate* isolate,
BuiltinArguments<BuiltinExtraArguments::kTarget> args) {
You could change this to generally accept kTargetAndNewTarget, then you don't
need to duplicate the code. Just make sure to also push NewTarget in
builtins-x64.cc

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc#newcode4494
src/builtins.cc:4494: DCHECK(args.receiver()->IsTheHole());
I wonder if we could unify this with the general implicit receiver construction.
If yes, then we could get away with only HandleApiCall and we wouldn't even need
the distinction in the JSConstructStubHelper.

https://codereview.chromium.org/1910253005/diff/1/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.h#newcode164
src/builtins.h:164: V(HandleApiCallConstruct, kTargetAndNewTarget)              
\
This is not sufficient, as the callsite is custom code in builtins-x64.cc and so
you need to make sure to manually add NewTarget there.

[Benedikt Meurer, 2016-04-23 18:21:45]

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc#newcode4481
src/builtins.cc:4481: Isolate* isolate,
BuiltinArguments<BuiltinExtraArguments::kTarget> args) {
On 2016/04/23 11:57:47, Benedikt Meurer wrote:
> You could change this to generally accept kTargetAndNewTarget, then you don't
> need to duplicate the code. Just make sure to also push NewTarget in
> builtins-x64.cc

Ignore the part about builtins-x64.cc, I just noticed that this will already
work.

https://codereview.chromium.org/1910253005/diff/1/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.h#newcode164
src/builtins.h:164: V(HandleApiCallConstruct, kTargetAndNewTarget)              
\
Actually this is nonsense. The JSConstructStubHelper passes NewTarget in the
appropriate register and the builtin adaptor pushes it, so everything works fine
the way it is currently.

[adamk, 2016-04-25 18:53:40]

Thanks for taking a look!

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc#newcode4481
src/builtins.cc:4481: Isolate* isolate,
BuiltinArguments<BuiltinExtraArguments::kTarget> args) {
On 2016/04/23 18:21:45, Benedikt Meurer wrote:
> On 2016/04/23 11:57:47, Benedikt Meurer wrote:
> > You could change this to generally accept kTargetAndNewTarget, then you
don't
> > need to duplicate the code. Just make sure to also push NewTarget in
> > builtins-x64.cc
> 
> Ignore the part about builtins-x64.cc, I just noticed that this will already
> work.

Can you point me at where we load undefined for the non-construct case? I see it
for ReflectApply and FunctionApply, but not in the general case...

[Benedikt Meurer, 2016-04-25 19:02:00]

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc#newcode4481
src/builtins.cc:4481: Isolate* isolate,
BuiltinArguments<BuiltinExtraArguments::kTarget> args) {
Sure, it's part of the invoke function sequence, see
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/macro-a...
for the x64 port.

[adamk, 2016-04-25 22:45:56]

Patchset #3 (id:40001) has been deleted

[adamk, 2016-04-25 22:59:46]

Description was changed from

==========
Work in progress: expose NewTarget to API functions

BUG=v8:4261
LOG=y
==========

to

==========
[api] Expose FunctionCallbackInfo::NewTarget

This is needed by Blink to implement the Custom Elements spec.

BUG=v8:4261
LOG=y
==========

[adamk, 2016-04-25 22:59:46]

adamk@chromium.org changed reviewers:
+ jochen@chromium.org

[adamk, 2016-04-25 22:59:55]

https://codereview.chromium.org/1910253005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1910253005/diff/1/include/v8.h#newcode3207
include/v8.h:3207: static const int kIsolateIndex = 1;
On 2016/04/23 11:45:41, Benedikt Meurer wrote:
> It'd be nice to remove isolate and callee here (not in the same CL, but
> soonish). The isolate can be retrieved from the holder (which is always a
> HeapObject), and the callee is deprecated for some time. 

Seems reasonable for a followup, indeed. My recollection is that Callee is on
life support, though, and can't yet be removed.

https://codereview.chromium.org/1910253005/diff/1/include/v8.h#newcode3222
include/v8.h:3222: int is_construct_call_;
On 2016/04/23 11:45:41, Benedikt Meurer wrote:
> This can now be removed (and potentially everything necessary to thread it
> through), as you can tell whether you were invoked via [[Call]] or
[[Construct]]
> by checking if NewTarget is undefined. And IsConstructCall() above can do
> exactly that.

There's one problem with removing this, which is the CallAsConstructor behavior
for non-Function callables (created via
ObjectTemplate::SetCallAsFunctionHandler). It's not clear what NewTarget()
should be in that case. I suppose we could set it as the object itself, but that
seems odd, as it's not "constructable" in the usual sense. Or I could set the
handler function, but that's not right either, at it was _not_ the new_target.
Thoughts?

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.cc#newcode4481
src/builtins.cc:4481: Isolate* isolate,
BuiltinArguments<BuiltinExtraArguments::kTarget> args) {
On 2016/04/23 11:57:47, Benedikt Meurer wrote:
> You could change this to generally accept kTargetAndNewTarget, then you don't
> need to duplicate the code. Just make sure to also push NewTarget in
> builtins-x64.cc

Done.

https://codereview.chromium.org/1910253005/diff/1/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/1910253005/diff/1/src/builtins.h#newcode164
src/builtins.h:164: V(HandleApiCallConstruct, kTargetAndNewTarget)              
\
On 2016/04/23 18:21:45, Benedikt Meurer wrote:
> Actually this is nonsense. The JSConstructStubHelper passes NewTarget in the
> appropriate register and the builtin adaptor pushes it, so everything works
fine
> the way it is currently.

Yup, this seems to work just fine.

[adamk, 2016-04-25 23:35:48]

https://codereview.chromium.org/1910253005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1910253005/diff/1/include/v8.h#newcode3222
include/v8.h:3222: int is_construct_call_;
On 2016/04/25 22:59:55, adamk wrote:
> On 2016/04/23 11:45:41, Benedikt Meurer wrote:
> > This can now be removed (and potentially everything necessary to thread it
> > through), as you can tell whether you were invoked via [[Call]] or
> [[Construct]]
> > by checking if NewTarget is undefined. And IsConstructCall() above can do
> > exactly that.
> 
> There's one problem with removing this, which is the CallAsConstructor
behavior
> for non-Function callables (created via
> ObjectTemplate::SetCallAsFunctionHandler). It's not clear what NewTarget()
> should be in that case. I suppose we could set it as the object itself, but
that
> seems odd, as it's not "constructable" in the usual sense. Or I could set the
> handler function, but that's not right either, at it was _not_ the new_target.
> Thoughts?

Patch set 4 includes the changes to IsConstructCall.

[Benedikt Meurer, 2016-04-26 04:15:34]

https://codereview.chromium.org/1910253005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1910253005/diff/1/include/v8.h#newcode3222
include/v8.h:3222: int is_construct_call_;
On 2016/04/25 22:59:55, adamk wrote:
> On 2016/04/23 11:45:41, Benedikt Meurer wrote:
> > This can now be removed (and potentially everything necessary to thread it
> > through), as you can tell whether you were invoked via [[Call]] or
> [[Construct]]
> > by checking if NewTarget is undefined. And IsConstructCall() above can do
> > exactly that.
> 
> There's one problem with removing this, which is the CallAsConstructor
behavior
> for non-Function callables (created via
> ObjectTemplate::SetCallAsFunctionHandler). It's not clear what NewTarget()
> should be in that case. I suppose we could set it as the object itself, but
that
> seems odd, as it's not "constructable" in the usual sense. Or I could set the
> handler function, but that's not right either, at it was _not_ the new_target.
> Thoughts?

You don't "set" new.target to something. You just pass whatever through whatever
was in rdx/edx/r3/whatever. The HandleApiCallAsConstructor has to thread through
this value.

[Benedikt Meurer, 2016-04-26 04:21:34]

Getting closer, looks like this allows for some awesome simplifications in
various places :-)

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:4731: HeapObject* new_target;
You don't need to make up a new.target here. Just change the two entry builtins
below to also thread through new.target in the args.

https://codereview.chromium.org/1910253005/diff/80001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:188: if (is_api_function) {
You shouldn't need this if anymore. The InvokeFunction below does exactly this
in case of an API function call.

[adamk, 2016-04-26 18:17:14]

Added ports, though the mips ones look a bit odd (I added TODOs where they look
bad).

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:4731: HeapObject* new_target;
On 2016/04/26 04:21:34, Benedikt Meurer wrote:
> You don't need to make up a new.target here. Just change the two entry
builtins
> below to also thread through new.target in the args.

Simply adding kNewTarget in builtins.h doesn't seem to work: undefined is passed
whether we're constructing or not. rdx gets clobbered by the CallFunction
builtin, which is what we jump to. See:

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/builtin...
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/builtin...

https://codereview.chromium.org/1910253005/diff/80001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:188: if (is_api_function) {
On 2016/04/26 04:21:34, Benedikt Meurer wrote:
> You shouldn't need this if anymore. The InvokeFunction below does exactly this
> in case of an API function call.

Done.

[Benedikt Meurer, 2016-04-26 18:28:41]

I think this is LGTM for the feature, thanks.

It would be nice to do some follow up cleanups, at least shrinking the
CallbackInfo by one word again (i.e. the isolate) should be easy. And maybe even
further unification of [[Construct]] handling for API things, although that's
not high priority.

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:4731: HeapObject* new_target;
Ah you're right. It's because we call... Meh, can you leave a TODO here, that we
should really handle [[Construct]] properly here as well at some point?

https://codereview.chromium.org/1910253005/diff/80001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:188: if (is_api_function) {
On 2016/04/26 18:17:14, adamk wrote:
> On 2016/04/26 04:21:34, Benedikt Meurer wrote:
> > You shouldn't need this if anymore. The InvokeFunction below does exactly
this
> > in case of an API function call.
> 
> Done.

Acknowledged.

[adamk, 2016-04-26 18:34:35]

Thanks for the review. I agree there's more cleanup to do here. Was "fun" to
learn about some of the dark corners here.

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1910253005/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:4731: HeapObject* new_target;
On 2016/04/26 18:28:40, Benedikt Meurer wrote:
> Ah you're right. It's because we call... Meh, can you leave a TODO here, that
we
> should really handle [[Construct]] properly here as well at some point?

Added a TODO.

[Benedikt Meurer, 2016-04-26 18:37:01]

On 2016/04/26 18:34:35, adamk wrote:
> Thanks for the review. I agree there's more cleanup to do here. Was "fun" to
> learn about some of the dark corners here.

Hah, dark corners... Dude, you should have look at it before the [[Call]] and
[[Construct]] refactorings late 2015. ;-)
This is already the bright side :-P

[adamk, 2016-04-26 18:54:23]

Jochen, does the additional API method look OK to you?

[jochen (gone - plz use gerrit), 2016-04-27 12:06:19]

lgtm

[adamk, 2016-04-27 18:03:57]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-04-27 18:03:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1910253005/#ps140001
(title: "Added a TODO")

[commit-bot: I haz the power, 2016-04-27 18:04:03]

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

[commit-bot: I haz the power, 2016-04-27 18:07:27]

Description was changed from

==========
[api] Expose FunctionCallbackInfo::NewTarget

This is needed by Blink to implement the Custom Elements spec.

BUG=v8:4261
LOG=y
==========

to

==========
[api] Expose FunctionCallbackInfo::NewTarget

This is needed by Blink to implement the Custom Elements spec.

BUG=v8:4261
LOG=y
==========

[commit-bot: I haz the power, 2016-04-27 18:07:28]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-05-01 22:01:27]

Description was changed from

==========
[api] Expose FunctionCallbackInfo::NewTarget

This is needed by Blink to implement the Custom Elements spec.

BUG=v8:4261
LOG=y
==========

to

==========
[api] Expose FunctionCallbackInfo::NewTarget

This is needed by Blink to implement the Custom Elements spec.

BUG=v8:4261
LOG=y

Committed: https://crrev.com/306c412ce04a7c21a208e33dfce530536453c20d
Cr-Commit-Position: refs/heads/master@{#35833}
==========

[commit-bot: I haz the power, 2016-05-01 22:01:28]

Patchset 7 (id:??) landed as
https://crrev.com/306c412ce04a7c21a208e33dfce530536453c20d
Cr-Commit-Position: refs/heads/master@{#35833}