bindings: Do not skip symbols in the JS -> record<> conversion.

bindings: Do not skip symbols in the JS -> record<> conversion.

We were previously skipping all symbols when calling GetOwnPropertyNames()
due to https://github.com/heycam/webidl/issues/294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu>; and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2900743002
Cr-Commit-Position: refs/heads/master@{#473905}
Committed: https://chromium.googlesource.com/chromium/src/+/0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b

[Raphael Kubo da Costa (rakuco), 2017-05-22 13:20:53]

PTAL

I can't trigger the bots yet because the required V8 change hasn't been rolled
in. I'd like to land this in a few days once I'm sure the V8 side sticks.

[Yuki, 2017-05-22 13:39:51]

I'm not quite sure about what our consensus is.  Do we all agree to throw? 
Boris wrote that no browser throws currently, and no other response is there yet
except for yours.

My questions are:
1. What is the best behavior from web developer's perspective?
2. Do we all agree on the spec?

I think that we can change the spec if necessary.  I vaguely guess that web
developers could be happier with the current implementation.

[Raphael Kubo da Costa (rakuco), 2017-05-23 09:43:17]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 09:43:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-05-23 09:58:48]

On 2017/05/22 13:39:51, Yuki wrote:
> I'm not quite sure about what our consensus is.  Do we all agree to throw? 
> Boris wrote that no browser throws currently, and no other response is there
yet
> except for yours.
> 
> My questions are:
> 1. What is the best behavior from web developer's perspective?
> 2. Do we all agree on the spec?
> 
> I think that we can change the spec if necessary.  I vaguely guess that web
> developers could be happier with the current implementation.

Does domenic's response in
https://github.com/heycam/webidl/issues/294#issuecomment-303184581 address all
your concerns?

[Yuki, 2017-05-23 10:32:37]

Yes, LGTM!

[commit-bot: I haz the power, 2017-05-23 11:26:13]

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

[commit-bot: I haz the power, 2017-05-23 11:26:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raphael Kubo da Costa (rakuco), 2017-05-23 12:46:53]

Hmm, some tests are failing because V8's GetOwnPropertyNames() does not convert
numeric properties to strings and the DCHECK() I added is failing. I'll see if I
can change V8.

[Yuki, 2017-05-23 13:22:41]

On 2017/05/23 12:46:53, Raphael Kubo da Costa (rakuco) wrote:
> Hmm, some tests are failing because V8's GetOwnPropertyNames() does not
convert
> numeric properties to strings and the DCHECK() I added is failing. I'll see if
I
> can change V8.

I'm not sure if it's good for us to change that part of V8.  We can easily tell
that a property key is numeric as seeing its type is v8::Number (or smi).  It
would be benefit for other use cases and/or for performance perspectives.

Can we just drop the DCHECK, which the spec does not require?  Or change the
condition to IsName() || IsNumber()?

[Raphael Kubo da Costa (rakuco), 2017-05-23 13:25:35]

I considered changing V8 (although it's a change in behavior) because
https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods
describes [[OwnPropertyKeys]] as returning only strings and symbols.

If you think it's better it's just as easy to change our own code.

[Raphael Kubo da Costa (rakuco), 2017-05-23 13:31:33]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[Raphael Kubo da Costa (rakuco), 2017-05-23 13:31:47]

On 2017/05/23 13:25:35, Raphael Kubo da Costa (rakuco) wrote:
> If you think it's better it's just as easy to change our own code.

Done in patch v3.

[commit-bot: I haz the power, 2017-05-23 13:31:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-05-23 13:32:04]

Description was changed from

==========
bindings: Do not skip symbols in the JS -> record<> conversion.

We were previously skipping all symbols when calling GetOwnPropertyNames()
due to https://github.com/heycam/webidl/issues/294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
bindings: Do not skip symbols in the JS -> record<> conversion.

We were previously skipping all symbols when calling GetOwnPropertyNames()
due to https://github.com/heycam/webidl/issues/294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

[Yuki, 2017-05-23 13:32:04]

On 2017/05/23 13:25:35, Raphael Kubo da Costa (rakuco) wrote:
> I considered changing V8 (although it's a change in behavior) because
>
https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods
> describes [[OwnPropertyKeys]] as returning only strings and symbols.
> 
> If you think it's better it's just as easy to change our own code.

I'm personally okay to change Blink's code in this CL, and it's nice to make it
clear that GetOwnPropertyNames() may return v8::Numbers by adding a comment in
v8.h.

I'm also fine that you'll consult with jochen@ or someone in V8 team.

I'm not quite sure if GetOwnPropertyNames() was meant to be exactly the same as
[[OwnPropertyKeys]] or not.

[Yuki, 2017-05-23 13:34:43]

LGTM.

[haraken, 2017-05-23 15:08:53]

LGTM

[commit-bot: I haz the power, 2017-05-23 15:16:14]

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

[commit-bot: I haz the power, 2017-05-23 15:16:15]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-05-23 15:16:42]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[commit-bot: I haz the power, 2017-05-23 15:17:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 15:20:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495552602344230,
"parent_rev": "297c1e59895cf9d34e9cb1fe7d3c457a650ac97a", "commit_rev":
"0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b"}

[commit-bot: I haz the power, 2017-05-23 15:21:11]

Description was changed from

==========
bindings: Do not skip symbols in the JS -> record<> conversion.

We were previously skipping all symbols when calling GetOwnPropertyNames()
due to https://github.com/heycam/webidl/issues/294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
bindings: Do not skip symbols in the JS -> record<> conversion.

We were previously skipping all symbols when calling GetOwnPropertyNames()
due to https://github.com/heycam/webidl/issues/294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2900743002
Cr-Commit-Position: refs/heads/master@{#473905}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fd77db4e8e5e8ad1ebdb2bd19bd...
==========

[commit-bot: I haz the power, 2017-05-23 15:21:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0fd77db4e8e5e8ad1ebdb2bd19bd...