bindings: Include symbols in record<> conversions |
||
Issue descriptionFrom https://github.com/heycam/webidl/issues/294: I think Gecko was the first engine to add support for WebIDL records. At the time, Boris asked what to do with enumerable symbols, since they'd just throw a type error when converted to a string type in step 4.2.1 of https://heycam.github.io/webidl/#es-record ("Let typedKey be key converted to an IDL value of type K"). At the time, Gecko chose to just ignore symbols and not enumerate them in the first place. When I added support for records in Blink, I chose to do the same because of the GitHub ticket above as well as to be compatible with Gecko. That ticket has since been reopened, Gecko is being changed to not ignore symbols and just let them throw exceptions as expected (https://bugzilla.mozilla.org/show_bug.cgi?id=1366032) and web-platform-tests will soon have some tests for this. I'm filing this issue for a couple of reasons: * To keep track of the analogous work here since it requires commits to both V8 and Blink. * To discuss whether the patches should be merged into M59 as well. M59 is the first milestone to include support for records, and doing so would ensure we don't change behavior between M59 and 60 (or 61).
,
May 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b5e610c19208ef854755eec67011ca7aff008bf4 commit b5e610c19208ef854755eec67011ca7aff008bf4 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Mon May 22 12:02:26 2017 Make Object::GetOwnPropertyDescriptor() take a Name, not a String. Most of the plumbing is already present in the non-public API. According to ES2016, Symbols are also accepted in calls to getOwnProperty(), and taking them is required in Blink for proper record<K,V> WebIDL conversions. R=jochen@chromium.org,verwaest@chromium.org Bug: chromium:724481 Change-Id: I0dfe0e57f6d811f04ecbfd8ec0c97e44c9f02c96 Reviewed-on: https://chromium-review.googlesource.com/509611 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com> Cr-Commit-Position: refs/heads/master@{#45454} [modify] https://crrev.com/b5e610c19208ef854755eec67011ca7aff008bf4/include/v8.h [modify] https://crrev.com/b5e610c19208ef854755eec67011ca7aff008bf4/src/api.cc [modify] https://crrev.com/b5e610c19208ef854755eec67011ca7aff008bf4/test/cctest/test-api.cc
,
May 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b commit 0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com> Date: Tue May 23 15:20:52 2017 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} [modify] https://crrev.com/0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b/third_party/WebKit/LayoutTests/external/wpt/fetch/api/headers/headers-record.html [modify] https://crrev.com/0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b/third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h [modify] https://crrev.com/0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b/third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImplTest.cpp
,
May 23 2017
These are the 2 commits we need to fix the issue. Given M60 will likely branch on the 25th, they are both going to make it. Is this M59 material as well?
,
May 23 2017
I think it's fine to not merge back
,
May 24 2017
Alright, M60 it is then! |
||
►
Sign in to add a comment |
||
Comment 1 by raphael....@intel.com
, May 22 2017