Update ObjectToString to Harmony-draft algorithm

Update ObjectToString to Harmony-draft algorithm

Updates Object.prototype.toString() to use algorithm described in harmony drafts.

Currently, the behaviour is essentially the same as ES262's version, however this changes when internal structures
such as Promise make use of symbolToStringTag (as they are supposed to, see v8:3241), and changes further once
Symbol.toStringTag is exposed publicly.

BUG=v8:3241, v8:3502
LOG=N
R=dslomov@chromium.org

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

[caitp (gmail), 2014-09-05 15:36:29]

Alternative version of https://codereview.chromium.org/227413004/ which uses the
proper Symbol implementation (although Symbol.toStringTag is still not exposed)
--- and implements the remainder of the Object.prototype.toString algorithm
specified in Harmony 19.1.3.6

[caitp (gmail), 2014-10-10 04:31:24]

Patchset #2 (id:20001) has been deleted

[caitp (gmail), 2014-10-10 04:34:02]

Adopt updated algorithm for Object.prototype.toString, which makes use of
Symbol.toStringTag --- expose symbolToStringTag via Symbol.toStringTag --- Add
<Type>.prototype [ @@toStringTag ] in a number of places as spec'd (with
exception of TypedArrays due to issues with macros)

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

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode217
src/v8natives.js:217: var kBuiltinStringTags = {
ObjectCreate(null) would be better, I guess

[caitp (gmail), 2014-10-16 19:53:47]

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

[rossberg, 2014-10-17 11:19:31]

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode217
src/v8natives.js:217: var kBuiltinStringTags = {
On 2014/10/10 14:56:28, caitp wrote:
> ObjectCreate(null) would be better, I guess

Yes, the prototype has to be null, otherwise the code below is wrong. You could
do "__proto__: null".

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode236
src/v8natives.js:236: var tag = O[symbolToStringTag];
We need some way of putting this new behaviour behind a flag first
(--harmony-tostring). You probably need to introduce a runtime function to query
the flag here.

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode237
src/v8natives.js:237: if (IS_UNDEFINED(tag)) tag = builtinTag;
Style nit: new line if there is an 'else'.

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode239
src/v8natives.js:239: if (!IS_STRING(tag)) return "[object ???]";
Same here. Can also merge into an 'else if' with the above 'else'.

https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/class-of-bu...
File test/mjsunit/class-of-builtins.js (right):

https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/class-of-bu...
test/mjsunit/class-of-builtins.js:31: /*Object:   [ Object ],*/
Why is this commented out?

https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/es6/promise...
File test/mjsunit/es6/promises.js (right):

https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/es6/promise...
test/mjsunit/es6/promises.js:64: (function() {
Move this to line 36.

[caitp (gmail), 2014-10-17 12:31:44]

On 2014/10/17 11:19:31, rossberg wrote:
> https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js
> File src/v8natives.js (right):
> 
>
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode217
> src/v8natives.js:217: var kBuiltinStringTags = {
> On 2014/10/10 14:56:28, caitp wrote:
> > ObjectCreate(null) would be better, I guess
> 
> Yes, the prototype has to be null, otherwise the code below is wrong. You
could
> do "__proto__: null".
> 
>
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode236
> src/v8natives.js:236: var tag = O[symbolToStringTag];
> We need some way of putting this new behaviour behind a flag first
> (--harmony-tostring). You probably need to introduce a runtime function to
query
> the flag here.
> 
>
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode237
> src/v8natives.js:237: if (IS_UNDEFINED(tag)) tag = builtinTag;
> Style nit: new line if there is an 'else'.
> 
>
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode239
> src/v8natives.js:239: if (!IS_STRING(tag)) return "[object ???]";
> Same here. Can also merge into an 'else if' with the above 'else'.
> 
>
https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/class-of-bu...
> File test/mjsunit/class-of-builtins.js (right):
> 
>
https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/class-of-bu...
> test/mjsunit/class-of-builtins.js:31: /*Object:   [ Object ],*/
> Why is this commented out?
> 
>
https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/es6/promise...
> File test/mjsunit/es6/promises.js (right):
> 
>
https://codereview.chromium.org/546803003/diff/70001/test/mjsunit/es6/promise...
> test/mjsunit/es6/promises.js:64: (function() {
> Move this to line 36.

Thanks for the prompt review! I will make these adjustments within an hour or
two =)

[arv (Not doing code reviews), 2014-10-17 14:40:50]

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

[arv (Not doing code reviews), 2014-10-17 14:40:51]

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode241
src/v8natives.js:241: return kBuiltinStringTags[tag];
Maybe make the kBuiltinStringTags a "set" and then concat the string on demand.
The usage of these seems very low so having the extra strings in memory does not
seem worth it.

var kBuiltinStringTags = {
  __proto__: null,
  "Arguments": true,
  "Array": true,
  "Boolean": true,
  "Date": true,
  "Error": true,
  "Function": true,
  "Number": true,
  "RegExp": true,
  "String": true,
};

...


} else if (tag !== builtinTag && kBuiltinStringTags[tag]) {
  return "[object ~" + tag + "]";
}

[caitp (gmail), 2014-10-17 18:05:57]

Moved to `harmony-tostring.js` loaded behind a flag, use null prototype for
kBuiltinStringTags, fixed tests and nits.

[arv (Not doing code reviews), 2014-10-17 18:22:26]

There is also ObjectProtoToString in C++. We should update that to match ES6 as
well.

[arv (Not doing code reviews), 2014-10-17 18:22:53]

On 2014/10/17 18:22:26, arv wrote:
> There is also ObjectProtoToString in C++. We should update that to match ES6
as
> well.

Which can be done in a different CL if you want.

[Dmitry Lomov (no reviews), 2014-10-17 19:29:05]

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

[Dmitry Lomov (no reviews), 2014-10-17 19:29:06]

Some comments

https://codereview.chromium.org/546803003/diff/240001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/546803003/diff/240001/src/flag-definitions.h#...
src/flag-definitions.h:165: DEFINE_BOOL(harmony_tostring, false, "enable ES6
version of Object.prototype.toString")
nit: 80 column lines

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js#newcode104
src/symbol.js:104: // TODO: Currently defined in harmony-tostring.js --- Move
here when shipping
nit: 80 column lines

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js#newcode115
src/symbol.js:115: $Symbol.prototype, symbolToStringTag, "Symbol", DONT_ENUM |
READ_ONLY);
Hmm, when --harmony-tostring is turned off, where symbolToStringTag comes from?

Maybe better to move all these prototype modifications to harmony-tostring.js?

https://codereview.chromium.org/546803003/diff/240001/test/mjsunit/es6/promis...
File test/mjsunit/es6/promises.js (right):

https://codereview.chromium.org/546803003/diff/240001/test/mjsunit/es6/promis...
test/mjsunit/es6/promises.js:28: // Flags: --allow-natives-syntax
--harmony-tostring
Instead of adding --harmony-tostring to each and every test,  maybe we can we
have one test for all toStrings for different types?

[caitp (gmail), 2014-10-17 20:12:13]

On 2014/10/17 18:22:53, arv wrote:
> On 2014/10/17 18:22:26, arv wrote:
> > There is also ObjectProtoToString in C++. We should update that to match ES6
> as
> > well.
> 
> Which can be done in a different CL if you want.

I've started on it, but the tests I wrote are crashing, and I'll probably need
some help implementing it in a way that isn't super slow (if this matters for
the API layer). It might be simpler to review that in a separate CL where fewer
files are touched.

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js#newcode104
src/symbol.js:104: // TODO: Currently defined in harmony-tostring.js --- Move
here when shipping
On 2014/10/17 19:29:06, Dmitry Lomov (chromium) wrote:
> nit: 80 column lines

Acknowledged.

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js#newcode115
src/symbol.js:115: $Symbol.prototype, symbolToStringTag, "Symbol", DONT_ENUM |
READ_ONLY);
On 2014/10/17 19:29:06, Dmitry Lomov (chromium) wrote:
> Hmm, when --harmony-tostring is turned off, where symbolToStringTag comes
from?
> 
> Maybe better to move all these prototype modifications to harmony-tostring.js?

It's defined in both harmony-tostring.js and symbol.js --- InternalSymbol will
just return the existing symbol in the well known symbols registry if it already
exists, so this is fine.

It doesn't hurt to define the property regardless of whether the flag is enabled
or not, although I guess it's a bit of extra memory that never gets used if it's
not there

https://codereview.chromium.org/546803003/diff/240001/test/mjsunit/es6/promis...
File test/mjsunit/es6/promises.js (right):

https://codereview.chromium.org/546803003/diff/240001/test/mjsunit/es6/promis...
test/mjsunit/es6/promises.js:28: // Flags: --allow-natives-syntax
--harmony-tostring
On 2014/10/17 19:29:06, Dmitry Lomov (chromium) wrote:
> Instead of adding --harmony-tostring to each and every test,  maybe we can we
> have one test for all toStrings for different types?

I thought about doing this originally, but this way seemed much simpler, so that
there's no need to worry about whether a different feature is behind a different
flag or not

[caitp (gmail), 2014-10-17 22:00:49]

Added the ObjectProtoToString stuff.

https://codereview.chromium.org/546803003/diff/260001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/546803003/diff/260001/src/api.cc#newcode3412
src/api.cc:3412: return v8::String::NewFromUtf8(isolate, "[object ~Arguments]");
I'm not sure if it's worth doing this linearly, or creating a way to do an O(1)
lookup instead --- I feel like creating tons of string handles + vectors is
probably not very good.

Let me know if theres's a better way to do this, please~

https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13505: CHECK(value->IsString() &&
value->Equals(v8_str("[object ~Array]")));
These tests are not very robust and don't cover the full gamut of cases... I can
add more if necessary.

[Dmitry Lomov (no reviews), 2014-10-18 08:41:49]

Some more comments.

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/546803003/diff/240001/src/symbol.js#newcode115
src/symbol.js:115: $Symbol.prototype, symbolToStringTag, "Symbol", DONT_ENUM |
READ_ONLY);
On 2014/10/17 20:12:13, caitp wrote:
> On 2014/10/17 19:29:06, Dmitry Lomov (chromium) wrote:
> > Hmm, when --harmony-tostring is turned off, where symbolToStringTag comes
> from?
> > 
> > Maybe better to move all these prototype modifications to
harmony-tostring.js?
> 
> It's defined in both harmony-tostring.js and symbol.js --- InternalSymbol will
> just return the existing symbol in the well known symbols registry if it
already
> exists, so this is fine.
Oh I see. missed that.
> 
> It doesn't hurt to define the property regardless of whether the flag is
enabled
> or not, although I guess it's a bit of extra memory that never gets used if
it's
> not there
Yes, it is fine

https://codereview.chromium.org/546803003/diff/240001/test/mjsunit/es6/promis...
File test/mjsunit/es6/promises.js (right):

https://codereview.chromium.org/546803003/diff/240001/test/mjsunit/es6/promis...
test/mjsunit/es6/promises.js:28: // Flags: --allow-natives-syntax
--harmony-tostring
On 2014/10/17 20:12:13, caitp wrote:
> On 2014/10/17 19:29:06, Dmitry Lomov (chromium) wrote:
> > Instead of adding --harmony-tostring to each and every test,  maybe we can
we
> > have one test for all toStrings for different types?
> 
> I thought about doing this originally, but this way seemed much simpler, so
that
> there's no need to worry about whether a different feature is behind a
different
> flag or not

Ok, I am convinced. It's a bit hard with those cross-cutting things

https://codereview.chromium.org/546803003/diff/260001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/546803003/diff/260001/src/api.cc#newcode3412
src/api.cc:3412: return v8::String::NewFromUtf8(isolate, "[object ~Arguments]");
On 2014/10/17 22:00:49, caitp wrote:
> I'm not sure if it's worth doing this linearly, or creating a way to do an
O(1)
> lookup instead --- I feel like creating tons of string handles + vectors is
> probably not very good.
> 
> Let me know if theres's a better way to do this, please~

Instead of IsUtf8EqualTo with a new string constant (this creates a new string
constant every time and does a real compare) you should do the same as above for
comparison with Arguments (line 3394): use i::String::Equals and internalized
strings.

https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13505: CHECK(value->IsString() &&
value->Equals(v8_str("[object ~Array]")));
On 2014/10/17 22:00:49, caitp wrote:
> These tests are not very robust and don't cover the full gamut of cases... I
can
> add more if necessary.

More tests is always good. Maybe you can reduce the boilerplate with
macros/templates

[Dmitry Lomov (no reviews), 2014-10-18 11:45:34]

Also, like Erik said, feel free to make a separate CL for ObjectProtoToString.

[caitp (gmail), 2014-10-18 13:34:35]

https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13505: CHECK(value->IsString() &&
value->Equals(v8_str("[object ~Array]")));
On 2014/10/18 08:41:49, Dmitry Lomov (chromium) wrote:
> On 2014/10/17 22:00:49, caitp wrote:
> > These tests are not very robust and don't cover the full gamut of cases... I
> can
> > add more if necessary.
> 
> More tests is always good. Maybe you can reduce the boilerplate with
> macros/templates

Yeah, most of the stuff being tested isn't intern'd yet, didn't want to add new
intern'd strings unless it was the way to go for it.

[Dmitry Lomov (no reviews), 2014-10-18 14:40:21]

On 2014/10/18 13:34:35, caitp wrote:
> https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:13505: CHECK(value->IsString() &&
> value->Equals(v8_str("[object ~Array]")));
> On 2014/10/18 08:41:49, Dmitry Lomov (chromium) wrote:
> > On 2014/10/17 22:00:49, caitp wrote:
> > > These tests are not very robust and don't cover the full gamut of cases...
I
> > can
> > > add more if necessary.
> > 
> > More tests is always good. Maybe you can reduce the boilerplate with
> > macros/templates
> 
> Yeah, most of the stuff being tested isn't intern'd yet, didn't want to add
new
> intern'd strings unless it was the way to go for it.

Most (all?) of the strings you need in that comparison are already interned for
you :) - see
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/heap.h...
You do not need to intern the string you return from the function.

[Dmitry Lomov (no reviews), 2014-10-18 15:16:48]

Getting there!
Now I reviewed the whole thing, sorry for giving out comments a bit
piecemeal-ish before - it is Saturday here  :) 

Just some more comments, mostly nits/extra tests, but one substantial one about
exceptions in api functions.

https://codereview.chromium.org/546803003/diff/280001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/546803003/diff/280001/src/api.cc#newcode3400
src/api.cc:3400: tag = i::Runtime::GetObjectProperty(
This calls into Javascript, so needs EXCEPTION_PREAMBLE/EXCEPTION_BAILOUT_CHECK.
(also add a test for for JS object that throws in toString handle. The code as
you wrote it will just crash if that happens)

https://codereview.chromium.org/546803003/diff/280001/src/api.cc#newcode3408
src/api.cc:3408: using namespace internal;
Nit: remove 'using namespace internal' here, we use i:: prefix everywhere.
Suggestion: extract all this tag checking logic into a helper function:
  bool GetPredefinedToString(Handle<String>* result)

https://codereview.chromium.org/546803003/diff/280001/src/api.cc#newcode3411
src/api.cc:3411: tag_name, i_isolate->factory()->Arguments_string())) {
Nit: introduce variable for i_isolate->factory()

https://codereview.chromium.org/546803003/diff/280001/src/generator.js
File src/generator.js (left):

https://codereview.chromium.org/546803003/diff/280001/src/generator.js#oldcode71
src/generator.js:71: 
Nit: keep this empty line.

https://codereview.chromium.org/546803003/diff/280001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/546803003/diff/280001/src/symbol.js#newcode104
src/symbol.js:104: // TODO: Currently defined in harmony-tostring.js --- Move
here when
Nit: I volunteer to own this todo, so add me here: TODO(dslomov):
(If you want, add yourself to the list too)

https://codereview.chromium.org/546803003/diff/280001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/280001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13505: CHECK(value->IsString() &&
value->Equals(v8_str("[object ~Array]")));
Add a test that creates an object with a getter for toStringTag. Actually two:
one that completes normally and one that throws. 

Add tests for all cases from 19.1.3.6.

https://codereview.chromium.org/546803003/diff/280001/test/mjsunit/es6/object...
File test/mjsunit/es6/object-tostring.js (right):

https://codereview.chromium.org/546803003/diff/280001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:7: // TODO: Move duplicated tests into
mjsunit/class-of-builtins.js
Nit: I volunteer to own this TODO, so add me to the list: TODO(dslomov):
Add yourself if you want to own it too :)

https://codereview.chromium.org/546803003/diff/280001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:10: /*  Function: [ Function ],
Why all these tests are commented out?

https://codereview.chromium.org/546803003/diff/280001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:33: assertEquals(f === "Object" ? "[object "
+ f + "]" : "[object ~" + f + "]",
Add tests for all cases from 19.1.3.6.

[Dmitry Lomov (no reviews), 2014-10-18 15:43:19]

One more process-y thing: even though this is not a large feature, it has its
own harmony flag etc, so I think this actually warrants going through the
process per https://developers.google.com/v8/launchprocess.

So you need to send an intent-to-implement e-mail to v8-users.
It is clearly long enough already in ES6 draft and spec has been stable for a
while.
This CL can be your design doc - just point to the code review url.

We should also check whether DevTools do the right thing for toString.

This CL covers conformance tests; we should think to what extent we need
performance tests for toString.

[caitp (gmail), 2014-10-18 20:23:36]

On 2014/10/18 15:43:19, Dmitry Lomov (chromium) wrote:
> One more process-y thing: even though this is not a large feature, it has its
> own harmony flag etc, so I think this actually warrants going through the
> process per https://developers.google.com/v8/launchprocess.
> 
> So you need to send an intent-to-implement e-mail to v8-users.
> It is clearly long enough already in ES6 draft and spec has been stable for a
> while.
> This CL can be your design doc - just point to the code review url.
> 
> We should also check whether DevTools do the right thing for toString.
> 
> This CL covers conformance tests; we should think to what extent we need
> performance tests for toString.

Apparently, InstallFunctions() / %AddNamedProperty will throw in debug builds if
the property already exists :( Had to change it a bit to work right in debug as
well as release.

There are now piles more tests, and I've moved the tag-checking into a separate
function.

[Dmitry Lomov (no reviews), 2014-10-18 21:24:19]

Almost there, only nits and just two (actually three) more tests.

https://codereview.chromium.org/546803003/diff/320001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/546803003/diff/320001/src/api.cc#newcode3443
src/api.cc:3443: if (GetPredefinedToString(tag_name, &result)) {
Nits:

Better formatting:

   if (GetPredefinedToString(tag_name, &result)) 
     return result;

   class_name = tag_name;

also above:
    if (!tag->IsUndefined() {
           if(!tag->IsString()) 
               return ....;
           ... // no else branch, code continues
    }

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
test/cctest/test-api.cc:2036: ApiTestFuzzer::Fuzz();
Why this line?

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13515: object = v8_compile("new " #type "()")->Run(); \
Nit: use CompileRun here

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate);
Great test, add one more where the getter is actually written in Javascript:

Local<v8::Value> obj = CompileRun(
          "o = {};"
          "Object.defineProperty(o, Symbol.toStringTag, { ... };"
          " o");

so that ObjectProtoToString actually executes some Javascript code.

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
File test/mjsunit/es6/object-tostring.js (right):

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:10: // class-of-builtins) when shipping.
I wouldn't even add this TODO here, extra tests are fine

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:20: Error:    [ Error, TypeError,
RangeError, SyntaxError, ReferenceError, EvalError, URIError ]
Nit: 80 line string

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:72: var obj = className === "Arguments" ?
Formatting is off

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:80: var obj = className === "Arguments" ?
Formatting is off

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:128: ].forEach(testToStringTagNonString);
Also add a test that check properties of Object.prototype.toString (should be
writeable, non-enumerable, configurable)

[Dmitry Lomov (no reviews), 2014-10-18 21:27:15]

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
File test/mjsunit/es6/object-tostring.js (right):

https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object...
test/mjsunit/es6/object-tostring.js:20: Error:    [ Error, TypeError,
RangeError, SyntaxError, ReferenceError, EvalError, URIError ]
s/line/column/

[caitp (gmail), 2014-10-18 21:27:40]

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate);
On 2014/10/18 21:24:19, Dmitry Lomov (chromium) wrote:
> Great test, add one more where the getter is actually written in Javascript:
> 
> Local<v8::Value> obj = CompileRun(
>           "o = {};"
>           "Object.defineProperty(o, Symbol.toStringTag, { ... };"
>           " o");
> 
> so that ObjectProtoToString actually executes some Javascript code.

There's a problem with that --- the bootstrapper didn't load harmony-tostring,
flipping the flag won't make it use the right function. I'm not sure if there's
a way to re-bootstrap mid-test or something.

[caitp (gmail), 2014-10-18 21:35:01]

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate);
Er, I mean `Symbol.toStringTag` is not exposed --- I could expose that easily,
but it still wouldn't use the right function --- I guess that doesn't matter for
the runtime tests though

[Dmitry Lomov (no reviews), 2014-10-18 21:45:00]

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate);
On 2014/10/18 21:27:40, caitp wrote:
> On 2014/10/18 21:24:19, Dmitry Lomov (chromium) wrote:
> > Great test, add one more where the getter is actually written in Javascript:
> > 
> > Local<v8::Value> obj = CompileRun(
> >           "o = {};"
> >           "Object.defineProperty(o, Symbol.toStringTag, { ... };"
> >           " o");
> > 
> > so that ObjectProtoToString actually executes some Javascript code.
> 
> There's a problem with that --- the bootstrapper didn't load harmony-tostring,
> flipping the flag won't make it use the right function. I'm not sure if
there's
> a way to re-bootstrap mid-test or something.
Ah, I see. Extract the tests for toStringTag into a separate test.
Use UNINITIALIZED_TEST macro for it instead of THREADED_TEST.
 Add 
    FLAG_harmony_tostring = true; 
    CcTest::InitializeVM();
at the beginning of your test.

[caitp (gmail), 2014-10-19 00:27:43]

On 2014/10/18 21:45:00, Dmitry Lomov (chromium) wrote:
> https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:13537: Local<v8::Object> obj =
v8::Object::New(isolate);
> On 2014/10/18 21:27:40, caitp wrote:
> > On 2014/10/18 21:24:19, Dmitry Lomov (chromium) wrote:
> > > Great test, add one more where the getter is actually written in
Javascript:
> > > 
> > > Local<v8::Value> obj = CompileRun(
> > >           "o = {};"
> > >           "Object.defineProperty(o, Symbol.toStringTag, { ... };"
> > >           " o");
> > > 
> > > so that ObjectProtoToString actually executes some Javascript code.
> > 
> > There's a problem with that --- the bootstrapper didn't load
harmony-tostring,
> > flipping the flag won't make it use the right function. I'm not sure if
> there's
> > a way to re-bootstrap mid-test or something.
> Ah, I see. Extract the tests for toStringTag into a separate test.
> Use UNINITIALIZED_TEST macro for it instead of THREADED_TEST.
>  Add 
>     FLAG_harmony_tostring = true; 
>     CcTest::InitializeVM();
> at the beginning of your test.

Interestingly, InitializeVM() will always crash if UNINITIALIZED_TEST() is used,
becaus InitializeVM() will invoke CcTest::isolate(), which asserts that isolate
is not null --- works with TEST() at least.

Will send out an intent-to-implement in a bit.

[Dmitry Lomov (no reviews), 2014-10-19 08:49:52]

LGTM with two more small nits.

https://codereview.chromium.org/546803003/diff/340001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/546803003/diff/340001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13513: CcTest::InitializeVM();
Ah, ok, then you don't need InitializeVM here - just move LocalContext
declaration here from line 13519.

https://codereview.chromium.org/546803003/diff/340001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13524: v8_compile("Object.prototype.toString =
function() {"
Nit: CompileRun here

https://codereview.chromium.org/546803003/diff/340001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13542: Local<Value> object = v8_compile("new
Object()")->Run();
Nit: CompileRun here

[Dmitry Lomov (no reviews), 2014-10-20 19:12:35]

Looks like you have not run 
   make quickcheck 
on this patch; if you do, test/webkit/fast/js/Promise-resolve-with-itself.js
fails.
I am fixing the underlying issue in https://codereview.chromium.org/649203004/,
so this cannot land before that lands.

(not sure why our trybots do not execute those tests; will follow up on that)

[caitp (gmail), 2014-10-20 19:25:44]

On 2014/10/20 19:12:35, Dmitry Lomov (chromium) wrote:
> Looks like you have not run 
>    make quickcheck 
> on this patch; if you do, test/webkit/fast/js/Promise-resolve-with-itself.js
> fails.
> I am fixing the underlying issue in
https://codereview.chromium.org/649203004/,
> so this cannot land before that lands.
> 
> (not sure why our trybots do not execute those tests; will follow up on that)

I did actually run the quick check build on x64.release before submitting the
last update, with only intl failures that always happen on my Mac. Just haven't
run the debug quick check

[caitp (gmail), 2014-10-21 01:21:02]

On 2014/10/20 19:25:44, caitp wrote:
> On 2014/10/20 19:12:35, Dmitry Lomov (chromium) wrote:
> > Looks like you have not run 
> >    make quickcheck 
> > on this patch; if you do, test/webkit/fast/js/Promise-resolve-with-itself.js
> > fails.
> > I am fixing the underlying issue in
> https://codereview.chromium.org/649203004/,
> > so this cannot land before that lands.
> > 
> > (not sure why our trybots do not execute those tests; will follow up on
that)
> 
> I did actually run the quick check build on x64.release before submitting the
> last update, with only intl failures that always happen on my Mac. Just
haven't
> run the debug quick check

Ping @dslomov, I've added an alternative fix for that failing test in this CL,
I'll try running all tests just to see if there are any other failures that
didn't run in my quickcheck run

[caitp (gmail), 2014-10-21 01:29:28]

yeah, with change all passing (except for a few intl issues that always fail for
me)

[Dmitry Lomov (no reviews), 2014-10-21 06:11:53]

Comments - we should make things safe.
Also, please rebase this patch (you will need some changes due to
https://code.google.com/p/v8/source/detail?r=24743

https://codereview.chromium.org/546803003/diff/380001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/546803003/diff/380001/src/messages.js#newcode226
src/messages.js:226: if (IS_OBJECT(obj) && %GetDataProperty(obj, "toString") ===
DefaultObjectToString) {
Line length

https://codereview.chromium.org/546803003/diff/380001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/546803003/diff/380001/src/v8natives.js#newcod...
src/v8natives.js:218: var DefaultObjectToString = ObjectToString;
Please rename ObjectToString to NoSideEffectsObjectToString.
We should continue using it instead of the ES6 one in bulitins even after we
ship the ES6 one.

[caitp (gmail), 2014-10-21 14:54:45]

Updated to use the new flag/experimentals machinery

https://codereview.chromium.org/546803003/diff/380001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/546803003/diff/380001/src/messages.js#newcode226
src/messages.js:226: if (IS_OBJECT(obj) && %GetDataProperty(obj, "toString") ===
DefaultObjectToString) {
On 2014/10/21 06:11:53, Dmitry Lomov (chromium) wrote:
> Line length

Acknowledged.

https://codereview.chromium.org/546803003/diff/380001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/546803003/diff/380001/src/v8natives.js#newcod...
src/v8natives.js:218: var DefaultObjectToString = ObjectToString;
On 2014/10/21 06:11:53, Dmitry Lomov (chromium) wrote:
> Please rename ObjectToString to NoSideEffectsObjectToString.
> We should continue using it instead of the ES6 one in bulitins even after we
> ship the ES6 one.

Acknowledged.

[Dmitry Lomov (no reviews), 2014-10-21 15:35:45]

lgtm, will land

[Dmitry Lomov (no reviews), 2014-10-21 17:22:01]

Committed patchset #11 (id:400001) manually as 24783 (presubmit successful).

[rossberg, 2014-10-22 06:41:57]

On 2014/10/21 17:22:01, Dmitry Lomov (chromium) wrote:
> Committed patchset #11 (id:400001) manually as 24783 (presubmit successful).

Did you run test262-es6? This seems to break it (S15.2.4.2_A6 failing).

[Dmitry Lomov (no reviews), 2014-10-22 07:59:21]

On 2014/10/22 06:41:57, rossberg wrote:
> On 2014/10/21 17:22:01, Dmitry Lomov (chromium) wrote:
> > Committed patchset #11 (id:400001) manually as 24783 (presubmit successful).
> 
> Did you run test262-es6? This seems to break it (S15.2.4.2_A6 failing).

I know I didn't. Caitlin caitp@, will you be able to look into this soon-ish?
Thanks!

[caitp (gmail), 2014-10-22 12:16:15]

On 2014/10/22 07:59:21, Dmitry Lomov (chromium) wrote:
> On 2014/10/22 06:41:57, rossberg wrote:
> > On 2014/10/21 17:22:01, Dmitry Lomov (chromium) wrote:
> > > Committed patchset #11 (id:400001) manually as 24783 (presubmit
successful).
> > 
> > Did you run test262-es6? This seems to break it (S15.2.4.2_A6 failing).
> 
> I know I didn't. Caitlin caitp@, will you be able to look into this soon-ish?
> Thanks!

Looks like a trivial fix, I'll submit a patch for it. I didn't know the ES5
tests were running for the test262-es6 run