Add remaining @@toStringTag symbols to builtins

Add remaining @@toStringTag symbols to builtins

R=dslomov@chromium.org

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

[caitp (gmail), 2014-10-23 17:28:14]

caitpotter88@gmail.com changed reviewers:
+ dslomov@chromium.org, rossberg@chromium.org

[caitp (gmail), 2014-10-23 17:29:51]

Adding the missing @@toStringTag items for builtin types.

There is a tiny spec violation WRT the name of the %TypedArray% @@toStringTag
getter --- SetFunctionName() wants a bit more work to do it correctly, so
there's a TODO in place to spend a bit more time making it set the name
properly.

[Dmitry Lomov (no reviews), 2014-10-24 08:20:48]

Some comments

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:70: static String* NameToFunctionName(Isolate*
isolate, Name* name) {
This function shouldn't use naked pointers - use Handle<Name>, Handle<String>
etc. I'll guide you with comments below

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:71: String* stringName = NULL;
Handle<String>

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:73: // TODO(caitp): Follow proper rules in
section 9.2.11 (SetFunctionName)
Yes, fine to do in later CL - our names for getters and setters builtins are
wrong anyway.

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:75: Object* symName =
Symbol::cast(name)->name();
Handle<Object> symName(Handle<Symbol>::cast(name)->name());

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:96: CONVERT_ARG_CHECKED(Name, name, 1);
CONVERT_ARG_HANDLE_CHECKED

In line 92 above, s/SealHandleScope/HandleScope/

https://codereview.chromium.org/664333003/diff/20001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/664333003/diff/20001/src/typedarray.js#newcod...
src/typedarray.js:304: //TypedArrayGetToStringTag
Stray comment?

[caitp (gmail), 2014-10-24 12:18:33]

Thanks --- Handle constructor doesn't like being called with a Handle, without
an isolate, apparently :>

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/664333003/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:70: static String* NameToFunctionName(Isolate*
isolate, Name* name) {
On 2014/10/24 08:20:47, Dmitry Lomov (chromium) wrote:
> This function shouldn't use naked pointers - use Handle<Name>, Handle<String>
> etc. I'll guide you with comments below

Done

[Dmitry Lomov (no reviews), 2014-10-24 12:38:55]

lgtm with small nits

https://codereview.chromium.org/664333003/diff/40001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/664333003/diff/40001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:89: HandleScope shs(isolate);
nit: s/shs/scope/

https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js#newcod...
src/typedarray.js:320: InstallGetter(global.NAME.prototype, symbolToStringTag,
TypedArrayGetToStringTag);
line length

[caitp (gmail), 2014-10-24 12:44:34]

https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js#newcod...
src/typedarray.js:298: return name;
admittedly the above if statement doesn't really make sense, it's just literally
following the spec --- fine to remove it

[caitp (gmail), 2014-10-24 14:23:14]

On 2014/10/24 12:44:34, caitp wrote:
> https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js
> File src/typedarray.js (right):
> 
>
https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js#newcod...
> src/typedarray.js:298: return name;
> admittedly the above if statement doesn't really make sense, it's just
literally
> following the spec --- fine to remove it

hmmm this is broken in debug, looking...

[caitp (gmail), 2014-10-24 15:11:19]

On 2014/10/24 14:23:14, caitp wrote:
> On 2014/10/24 12:44:34, caitp wrote:
> > https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js
> > File src/typedarray.js (right):
> > 
> >
>
https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js#newcod...
> > src/typedarray.js:298: return name;
> > admittedly the above if statement doesn't really make sense, it's just
> literally
> > following the spec --- fine to remove it
> 
> hmmm this is broken in debug, looking...

ExtractAccessorPairProperty() would try to cast to a String() when calling
SetPropertyReference() --- but SetPropertyReference() actually takes a Name,
which should work for Symbol getters/setters too. So it's a pretty trivial
change to fix those tests.

[Dmitry Lomov (no reviews), 2014-10-24 16:34:13]

On 2014/10/24 15:11:19, caitp wrote:
> On 2014/10/24 14:23:14, caitp wrote:
> > On 2014/10/24 12:44:34, caitp wrote:
> > > https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js
> > > File src/typedarray.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/664333003/diff/40001/src/typedarray.js#newcod...
> > > src/typedarray.js:298: return name;
> > > admittedly the above if statement doesn't really make sense, it's just
> > literally
> > > following the spec --- fine to remove it
> > 
> > hmmm this is broken in debug, looking...
> 
> ExtractAccessorPairProperty() would try to cast to a String() when calling
> SetPropertyReference() --- but SetPropertyReference() actually takes a Name,
> which should work for Symbol getters/setters too. So it's a pretty trivial
> change to fix those tests.

lgtm

[Dmitry Lomov (no reviews), 2014-10-24 19:28:43]

Committed patchset #5 (id:80001) manually as 24885 (presubmit successful).