[builtins] Also simplify the Symbol constructor.

[builtins] Also simplify the Symbol constructor.

No need to rely on the %_IsConstructCall magic here, we can just
implement the Symbol constructor in C++ altogether (it was just a
stupid wrapper around %CreateSymbol anyway).

R=mstarzinger@chromium.org

Committed: https://crrev.com/04087a7e45648a41b791e335ca852e53676f11f8
Cr-Commit-Position: refs/heads/master@{#30762}

[Benedikt Meurer, 2015-09-16 10:15:40]

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

[commit-bot: I haz the power, 2015-09-16 10:15:45]

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

[commit-bot: I haz the power, 2015-09-16 10:26:29]

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

[commit-bot: I haz the power, 2015-09-16 10:26:30]

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

[Michael Starzinger, 2015-09-16 11:33:04]

LGTM.

[Benedikt Meurer, 2015-09-16 11:33:25]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-09-16 11:33:38]

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

[commit-bot: I haz the power, 2015-09-16 11:35:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-16 11:35:42]

Patchset 2 (id:??) landed as
https://crrev.com/04087a7e45648a41b791e335ca852e53676f11f8
Cr-Commit-Position: refs/heads/master@{#30762}

[adamk, 2015-09-16 19:33:31]

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

[adamk, 2015-09-16 19:33:31]

https://codereview.chromium.org/1349643002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1349643002/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1475: ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, description,
I'd like to understand why this approach is better than the previous version.
Here the call stack is:

JS -> C++ -> JS

instead of the old

JS -> JS

I thought I'd understood that part of the point of these cleanups was to reduce
the hops between JS and C++, but that seems to be increased by this change.

If the main point of this change is to get rid of %_IsConstructCall(), you could
replace it with new.target instead, which in ES6 exposes the IsConstruct-ness of
a [[Call]] to author script.

[Benedikt Meurer, 2015-09-17 02:58:10]

https://codereview.chromium.org/1349643002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1349643002/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1475: ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, description,
On 2015/09/16 19:33:31, adamk wrote:
> I'd like to understand why this approach is better than the previous version.
> Here the call stack is:
> 
> JS -> C++ -> JS
> 
> instead of the old
> 
> JS -> JS
> 
> I thought I'd understood that part of the point of these cleanups was to
reduce
> the hops between JS and C++, but that seems to be increased by this change.
> 
> If the main point of this change is to get rid of %_IsConstructCall(), you
could
> replace it with new.target instead, which in ES6 exposes the IsConstruct-ness
of
> a [[Call]] to author script.

The main point of these cleanups is to make it correct, and as you guessed,
%_IsConstructCall is buggy (in the case of TCO), and unfortunately the same
applies to new.target (currently). But in case of the Symbol constructor it was
never

 JS -> JS 

but always

 JS -> (JS ->) C++ -> JS

because it always ended up with a call to %CreateSymbol, and it was also using
the obsolete ToString implementation, which is why it wouldn't do the callback
from C++ to JS previously. So fixing all of that was easiest by just writing
everything in C++, and that's what we agreed on as baseline implementation. If
that ever becomes a bottleneck we can turn it into a native builtin.