Mirror object properties are always names

Mirror object properties are always names

R=aandrey@chromium.org, rossberg@chromium.org, yangguo@chromium.org
BUG=

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

[wingo, 2014-08-06 14:14:50]

PTAL.  This needs to be fixed in order to ship iterators; otherwise symbolic
properties cause the mirror serializer to bail.  Even with this patch they
aren't serialized nicely.  Not sure what impact on Chromium is; are they
expecting indexed properties to be numbers and not names?

[Yang, 2014-08-06 14:20:20]

I don't get how this affects iterators. Why do we need index property names to
be strings? Could you elaborate in more detail please?

Adding Andrey from the DevTools team.

[rossberg, 2014-08-06 14:21:30]

https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js#newco...
src/mirror-debugger.js:762: names[index++] = %ToString(elementNames[i]);
Hm, I don't understand why this change is necessary.

https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js#newco...
src/mirror-debugger.js:800: name = IS_SYMBOL(name) ? name : %ToString(name);
You can use %ToName for this.

[wingo, 2014-08-06 14:22:47]

On 2014/08/06 14:20:20, Yang wrote:
> I don't get how this affects iterators. Why do we need index property names to
> be strings? Could you elaborate in more detail please?
> 
> Adding Andrey from the DevTools team.

Because enabling iterators adds a Symbol.iterator property on Array.prototype. 
Without this patch, we fail in ObjectMirror.prototype.property as the symbol is
passed to %ToString.

[wingo, 2014-08-06 14:24:54]

On 2014/08/06 14:21:30, rossberg wrote:
> https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js
> File src/mirror-debugger.js (right):
> 
>
https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js#newco...
> src/mirror-debugger.js:762: names[index++] = %ToString(elementNames[i]);
> Hm, I don't understand why this change is necessary.

The documentation for PropertyMirror claims that name is a string (which I
generalized to Name in this patch).  There is also the helper method isIndexed. 
%DebugGetPropertyDetails bails if the passed property is not a name.  All of
these lead me to believe that at one point the design was that "name" should be
a name and not a number.  I am ignorant, though, about the direction that this
code is moving.

>
https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js#newco...
> src/mirror-debugger.js:800: name = IS_SYMBOL(name) ? name : %ToString(name);
> You can use %ToName for this.

Neat, will fix.

[wingo, 2014-08-06 14:44:23]

>
https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js#newco...
> > src/mirror-debugger.js:800: name = IS_SYMBOL(name) ? name : %ToString(name);
> > You can use %ToName for this.
> 
> Neat, will fix.

This could be the minimal fix, FWIW.  Probably best as we don't need to get
feedback from devtools folks to make this change.

[rossberg, 2014-08-06 14:46:40]

On 2014/08/06 14:44:23, wingo wrote:
> >
>
https://codereview.chromium.org/443843004/diff/1/src/mirror-debugger.js#newco...
> > > src/mirror-debugger.js:800: name = IS_SYMBOL(name) ? name :
%ToString(name);
> > > You can use %ToName for this.
> > 
> > Neat, will fix.
> 
> This could be the minimal fix, FWIW.  Probably best as we don't need to get
> feedback from devtools folks to make this change.

Yes, I'd go with only this change for now. The debugger is brittle and poorly
understood, don't touch more than absolutely necessary... :)

[wingo, 2014-08-06 15:34:33]

Updated patch has minimal change to not throw on symbol properties.

[rossberg, 2014-08-06 15:54:34]

LGTM, modulo the comment

https://codereview.chromium.org/443843004/diff/20001/test/mjsunit/mirror-obje...
File test/mjsunit/mirror-object.js (right):

https://codereview.chromium.org/443843004/diff/20001/test/mjsunit/mirror-obje...
test/mjsunit/mirror-object.js:116: // Serialization of symbol-named properties
to JSON doesn't really
This isn't ever supposed to work; symbol properties are intentionally ignored by
JSON, like anything else it cannot represent.

[aandrey, 2014-08-06 16:04:55]

lgtm

[wingo, 2014-08-07 08:26:57]

On 2014/08/06 15:54:34, rossberg wrote:
> LGTM, modulo the comment
> 
>
https://codereview.chromium.org/443843004/diff/20001/test/mjsunit/mirror-obje...
> File test/mjsunit/mirror-object.js (right):
> 
>
https://codereview.chromium.org/443843004/diff/20001/test/mjsunit/mirror-obje...
> test/mjsunit/mirror-object.js:116: // Serialization of symbol-named properties
> to JSON doesn't really
> This isn't ever supposed to work; symbol properties are intentionally ignored
by
> JSON, like anything else it cannot represent.

Not sure what you mean here.  The symbol properties do get serialized to JSON;
the JSON entry simply has no name property (!) so you can't know which unnamed
property corresponds to which symbol.  In this way you can see that an object
has a symbol-valued property, and inspect the value, you just can't know what
symbol it is.  So, the comment does not seem to me to be misleading.

[rossberg, 2014-08-07 08:33:52]

On 2014/08/07 08:26:57, wingo wrote:
> On 2014/08/06 15:54:34, rossberg wrote:
> > LGTM, modulo the comment
> > 
> >
>
https://codereview.chromium.org/443843004/diff/20001/test/mjsunit/mirror-obje...
> > File test/mjsunit/mirror-object.js (right):
> > 
> >
>
https://codereview.chromium.org/443843004/diff/20001/test/mjsunit/mirror-obje...
> > test/mjsunit/mirror-object.js:116: // Serialization of symbol-named
properties
> > to JSON doesn't really
> > This isn't ever supposed to work; symbol properties are intentionally
ignored
> by
> > JSON, like anything else it cannot represent.
> 
> Not sure what you mean here.  The symbol properties do get serialized to JSON;
> the JSON entry simply has no name property (!) so you can't know which unnamed
> property corresponds to which symbol.  In this way you can see that an object
> has a symbol-valued property, and inspect the value, you just can't know what
> symbol it is.  So, the comment does not seem to me to be misleading.

Okay, I wasn't aware that our JSON implementation does s.th weird with symbol
properties. It should really just ignore them. I'll look into that.

[wingo, 2014-08-07 08:36:48]

Committed patchset #2 manually as 22959 (presubmit successful).