Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784543004:
Expose %IteratorPrototype% as an intrinsic in the public API. (Closed)
|
Created:
3 years, 8 months ago by Raphael Kubo da Costa (rakuco) Modified:
3 years, 8 months ago CC:
bashi, v8-reviews_googlegroups.com, Yuki Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionExpose %IteratorPrototype% as an intrinsic in the public API.
The WebIDL spec expects iterator objects from interfaces that declare pair
iterators to ultimately inherit from %IteratorPrototype%. Expose the
intrinsic object in the public API so we can use it in Blink's bindings
code.
BUG=chromium:689576
R=caitp@igalia.com,jkummerow@chromium.org,jochen@chromium.org
Review-Url: https://codereview.chromium.org/2784543004
Cr-Commit-Position: refs/heads/master@{#44472}
Committed: https://chromium.googlesource.com/v8/v8/+/5ec1cddcdd0c599b00ae8f4fbb54987f92867b12
Patch Set 1 #Patch Set 2 : Do not use deprecated API calls in the tests #
Total comments: 1
Messages
Total messages: 29 (14 generated)
The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expose %IteratorPrototype% as an intrinsic in the public API. The WebIDL spec expects iterator objects from interfaces that declare pair iterators to ultimately inherit from %IteratorPrototype%. Expose the intrinsic object in the public API so we can use it in Blink's bindings code. BUG=chromium:689576 R=caitp@igalia.com,jkummerow@chromium.org,jochen@chromium.org ========== to ========== Expose %IteratorPrototype% as an intrinsic in the public API. The WebIDL spec expects iterator objects from interfaces that declare pair iterators to ultimately inherit from %IteratorPrototype%. Expose the intrinsic object in the public API so we can use it in Blink's bindings code. BUG=chromium:689576 R=caitp@igalia.com,jkummerow@chromium.org,jochen@chromium.org ==========
PTAL. I kind of feel like I'm abusing the function template API to achieve what I need in WebIDL (see the "Put %IteratorProto% in a function object's inheritance chain" test), but I couldn't figure out any other way to do this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2784543004/diff/20001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/2784543004/diff/20001/test/cctest/test-api.cc... test/cctest/test-api.cc:25942: + parent_template->RemovePrototype(); // Remove so there is no name clash. This will force the instantiated Function to be non-constructible, which I guess is fine for most DOM interfaces, but still a bit strange. I agree there doesn't appear to be much else you can do in the current API.
+verwaest
raphael.kubo.da.costa@intel.com changed reviewers: + verwaest@chromium.org
+Toon, now in the right field.
ping?
jochen@chromium.org changed reviewers: + haraken@chromium.org
+haraken for bindings insight. DOM interfaces typically are constructible (albeit not necessarily from javascript), so I'm not sure whether this change will have the desired effect
On 2017/04/05 08:20:13, jochen wrote: > +haraken for bindings insight. > > DOM interfaces typically are constructible (albeit not necessarily from > javascript), so I'm not sure whether this change will have the desired effect What do you mean? c.f., the binding side CL is https://codereview.chromium.org/2777183004/
On 2017/04/05 at 08:22:16, haraken wrote: > On 2017/04/05 08:20:13, jochen wrote: > > +haraken for bindings insight. > > > > DOM interfaces typically are constructible (albeit not necessarily from > > javascript), so I'm not sure whether this change will have the desired effect > > What do you mean? > > c.f., the binding side CL is https://codereview.chromium.org/2777183004/ see the comment on the api test, apparently the prototype of the template needs to be removed to make the test pass
On 2017/04/05 08:43:24, jochen wrote: > On 2017/04/05 at 08:22:16, haraken wrote: > > On 2017/04/05 08:20:13, jochen wrote: > > > +haraken for bindings insight. > > > > > > DOM interfaces typically are constructible (albeit not necessarily from > > > javascript), so I'm not sure whether this change will have the desired > effect > > > > What do you mean? > > > > c.f., the binding side CL is https://codereview.chromium.org/2777183004/ > > see the comment on the api test, apparently the prototype of the template needs > to be removed to make the test pass Maybe I'm still missing out on something, but it's only the parent template that's having its prototype removed. It's never used anywhere directly and it's only created to hold a "prototype" property that will be accessed by the other template's Inherit() + GetFunction() combination. This other template is the one that's really used and exposed, and it continues to have a sane prototype chain.
if Kentaro is happy with this API behavior, it's fine by me. However, can we please add tests on the blink side as well? I'd expect some web platform tests for this, so we can verify that we match other browser's implementation
On 2017/04/05 13:44:54, jochen wrote: > if Kentaro is happy with this API behavior, it's fine by me. However, can we > please add tests on the blink side as well? I'd expect some web platform tests > for this, so we can verify that we match other browser's implementation I agree that the API looks tricky but IIUC this CL will give us a correct behavior. No object is instantiated from the parent template, so removing the prototype chain from the parent template is fine. Unless there's no easier way to define the "prototype" intrinsic property, this CL LGTM.
Could an OWNER also post an l-g-t-m following haraken's?
lgtm
The CQ bit was checked by raphael.kubo.da.costa@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491551044430250,
"parent_rev": "1329d15e99096559a41ac7cb49fbd0199607a5d2", "commit_rev":
"5ec1cddcdd0c599b00ae8f4fbb54987f92867b12"}
Message was sent while issue was closed.
Description was changed from ========== Expose %IteratorPrototype% as an intrinsic in the public API. The WebIDL spec expects iterator objects from interfaces that declare pair iterators to ultimately inherit from %IteratorPrototype%. Expose the intrinsic object in the public API so we can use it in Blink's bindings code. BUG=chromium:689576 R=caitp@igalia.com,jkummerow@chromium.org,jochen@chromium.org ========== to ========== Expose %IteratorPrototype% as an intrinsic in the public API. The WebIDL spec expects iterator objects from interfaces that declare pair iterators to ultimately inherit from %IteratorPrototype%. Expose the intrinsic object in the public API so we can use it in Blink's bindings code. BUG=chromium:689576 R=caitp@igalia.com,jkummerow@chromium.org,jochen@chromium.org Review-Url: https://codereview.chromium.org/2784543004 Cr-Commit-Position: refs/heads/master@{#44472} Committed: https://chromium.googlesource.com/v8/v8/+/5ec1cddcdd0c599b00ae8f4fbb54987f928... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/5ec1cddcdd0c599b00ae8f4fbb54987f928... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
