[runtime] [proxy] Implementing [[Call]]

[runtime] [proxy] Implementing [[Call]]

BUG=v8:1543
LOG=N

Committed: https://crrev.com/72994124733215c15ff0022616949a2291f3c26c
Cr-Commit-Position: refs/heads/master@{#32675}

[Camillo Bruni, 2015-12-04 15:52:15]

cbruni@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Camillo Bruni, 2015-12-04 15:52:16]

WIP, 
I have an issue in the proxies-call.js test, the arguments array I create for
the proxy trap (see runtime-proxy.cc) is not a Array but rather an Object when
inspected in JS.

[Camillo Bruni, 2015-12-07 10:21:21]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 10:21:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499593003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499593003/160001

[commit-bot: I haz the power, 2015-12-07 10:33:49]

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

[commit-bot: I haz the power, 2015-12-07 10:33:50]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/12799)

[Camillo Bruni, 2015-12-07 11:29:34]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 11:29:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499593003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499593003/180001

[commit-bot: I haz the power, 2015-12-07 11:32:06]

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

[commit-bot: I haz the power, 2015-12-07 11:32:07]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Camillo Bruni, 2015-12-07 13:14:26]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 13:14:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499593003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499593003/200001

[Camillo Bruni, 2015-12-07 13:18:05]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 13:18:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499593003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499593003/220001

[Camillo Bruni, 2015-12-07 13:26:23]

PTAL

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
File src/runtime/runtime-proxy.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:18: RUNTIME_FUNCTION(Runtime_JSProxyCall) {
Would it make sense to create a builtin instead? We well not call this from
JS...

[commit-bot: I haz the power, 2015-12-07 13:40:15]

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

[commit-bot: I haz the power, 2015-12-07 13:40:16]

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2015-12-07 15:55:08]

LGTM with a bunch of minor comments.

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2153: Handle<String> name =
factory->InternalizeUtf8String("Proxy");
"Proxy" should probably be in the INTERNALIZED_STRING_LIST in heap.h.

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2154: Handle<String> name_string =
Name::ToFunctionName(name).ToHandleChecked();
This doesn't do anything, feel free to drop it.

https://codereview.chromium.org/1499593003/diff/220001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/objects.cc#newcode...
src/objects.cc:1000: ASSIGN_RETURN_ON_EXCEPTION(isolate, target_proto,
Why did existing tests not catch this? Maybe add another test?

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
File src/runtime/runtime-proxy.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:17: // 9.5.13 [[Call]] (thisArgument,
argumentsList)
nit: include "ES6" please

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:18: RUNTIME_FUNCTION(Runtime_JSProxyCall) {
On 2015/12/07 13:26:23, cbruni wrote:
> Would it make sense to create a builtin instead? We well not call this from
> JS...

A runtime function is fine, I think.

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:21: DCHECK_LE(1, args.length());
s/1/2/, judging from the "- 2" above.

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:36: Handle<JSReceiver>
target(JSReceiver::cast(proxy->target()), isolate);
nit: cast is unnecessary, proxy->target() returns a JSReceiver*.

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-call.js (right):

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-call.js:61: seen_receiver  = this;
super-nit: s/  / /

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-call.js:65: Reflect.apply(proxy, apply_receiver ,
[1,2,3,4]);
nit: s/ ,/,/

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-call.js:78: target.apply(this_arg, args);
You probably want to check that this, too, was called (as opposed to the
existing trap being ignored). Maybe turn |called| into a counter, put "called +=
10" and "called += 1" into |target| and |handler.apply| respectively, and then
assertEquals(11, called).

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-call.js:83: Reflect.apply(proxy, {rec:1},
[1,2,3,4]);
I'd align the arguments with the assertEquals() in |target|. Either pass in only
[1, 2], or check all four.

[Toon Verwaest, 2015-12-07 16:32:57]

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

[Toon Verwaest, 2015-12-07 16:32:58]

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2161: proxy_fun->map()->set_is_constructor(true);
this turns all sloppy functions without prototype into constructors :(

[Camillo Bruni, 2015-12-07 16:54:10]

hpayer: please do the heap.

verwaest, jkummerow: addressed comments and nits

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2153: Handle<String> name =
factory->InternalizeUtf8String("Proxy");
On 2015/12/07 at 15:55:07, Jakob wrote:
> "Proxy" should probably be in the INTERNALIZED_STRING_LIST in heap.h.

indeed, better.

https://codereview.chromium.org/1499593003/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2161: proxy_fun->map()->set_is_constructor(true);
On 2015/12/07 at 16:32:57, Toon Verwaest wrote:
> this turns all sloppy functions without prototype into constructors :(

no no, it wont :(... changed

https://codereview.chromium.org/1499593003/diff/220001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/objects.cc#newcode...
src/objects.cc:1000: ASSIGN_RETURN_ON_EXCEPTION(isolate, target_proto,
On 2015/12/07 at 15:55:07, Jakob wrote:
> Why did existing tests not catch this? Maybe add another test?

kept a note. will do in a separate cl.

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
File src/runtime/runtime-proxy.cc (right):

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:21: DCHECK_LE(1, args.length());
On 2015/12/07 at 15:55:08, Jakob wrote:
> s/1/2/, judging from the "- 2" above.

this is actually the size of the wrapper arguments array used further down (a
bit confusing).
Moved down to where it's used the first time.

https://codereview.chromium.org/1499593003/diff/220001/src/runtime/runtime-pr...
src/runtime/runtime-proxy.cc:36: Handle<JSReceiver>
target(JSReceiver::cast(proxy->target()), isolate);
On 2015/12/07 at 15:55:08, Jakob wrote:
> nit: cast is unnecessary, proxy->target() returns a JSReceiver*.

right.

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-call.js (right):

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-call.js:78: target.apply(this_arg, args);
On 2015/12/07 at 15:55:08, Jakob wrote:
> You probably want to check that this, too, was called (as opposed to the
existing trap being ignored). Maybe turn |called| into a counter, put "called +=
10" and "called += 1" into |target| and |handler.apply| respectively, and then
assertEquals(11, called).

added called_target and called_handler

https://codereview.chromium.org/1499593003/diff/220001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-call.js:83: Reflect.apply(proxy, {rec:1},
[1,2,3,4]);
On 2015/12/07 at 15:55:08, Jakob wrote:
> I'd align the arguments with the assertEquals() in |target|. Either pass in
only [1, 2], or check all four.

right.

[Jakob Kummerow, 2015-12-07 17:07:24]

patch set 16 LGTM

[Camillo Bruni, 2015-12-07 17:44:32]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 17:44:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499593003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499593003/340001

[commit-bot: I haz the power, 2015-12-07 18:21:51]

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

[commit-bot: I haz the power, 2015-12-07 18:21:52]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-12-07 18:54:41]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-12-07 18:54:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1499593003/#ps340001
(title: "updating comment")

[commit-bot: I haz the power, 2015-12-07 18:54:46]

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

[commit-bot: I haz the power, 2015-12-07 18:59:16]

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

[commit-bot: I haz the power, 2015-12-07 18:59:17]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/8622)

[Camillo Bruni, 2015-12-08 10:42:46]

cbruni@chromium.org changed reviewers:
+ hpayer@chromium.org

[Hannes Payer (out of office), 2015-12-08 14:11:01]

heap lgtm

[Camillo Bruni, 2015-12-08 14:14:12]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2015-12-08 14:14:25]

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

[Camillo Bruni, 2015-12-08 15:06:56]

The CQ bit was unchecked by cbruni@chromium.org

[Camillo Bruni, 2015-12-08 15:44:42]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2015-12-08 15:44:50]

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

[commit-bot: I haz the power, 2015-12-08 16:04:15]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-12-08 16:05:18]

Description was changed from

==========
[runtime] [proxy] Implementing [[Call]]

BUG=v8:1543
LOG=N
==========

to

==========
[runtime] [proxy] Implementing [[Call]]

BUG=v8:1543
LOG=N

Committed: https://crrev.com/72994124733215c15ff0022616949a2291f3c26c
Cr-Commit-Position: refs/heads/master@{#32675}
==========

[commit-bot: I haz the power, 2015-12-08 16:05:19]

Patchset 18 (id:??) landed as
https://crrev.com/72994124733215c15ff0022616949a2291f3c26c
Cr-Commit-Position: refs/heads/master@{#32675}