New issue
Advanced search Search tips

Issue 828401 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 839389



Sign in to add a comment

Auto-generated WebIDL bindings are unable to express certain overloaded functions

Project Member Reported by hbos@chromium.org, Apr 3 2018

Issue description

RTCPeerConnection.idl has:

// Legacy API
[CallWith=ScriptState, LegacyInterfaceTypeChecking] Promise<void> getStats(RTCStatsCallback successCallback, optional MediaStreamTrack? selector);

But when overloading this to also support:

// Spec-compliant API
// https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getstats
[CallWith=ScriptState] Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null);

We end up with TypeErrors upon supplying valid arguments, the wrong version being called or compile errors for generated functions that are unused. I've tried various different combinations, and ended up having to implement custom bindings code for this in RTCPeerConnection.cpp:
https://chromium-review.googlesource.com/c/chromium/src/+/978128
 
Components: -Infra>Client>V8 Infra>Client>WebRTC
Maybe you meant webrtc component?

Comment 2 by hbos@chromium.org, Apr 4 2018

This use case is WebRTC but the bug is in V8 bindings. What component to use when there is a bug in the V8Foo.[h/cpp] files auto-generated from Foo.idl files?

Comment 3 by hbos@chromium.org, Apr 4 2018

(This could hit anybody overloading functions in .idl, not specific to WebRTC)

Comment 4 by hbos@chromium.org, Apr 4 2018

I put this as low priority because it is a rare type of overload and I found a workaround, but the bug should be triaged by V8 magicians.
Components: -Infra>Client>WebRTC Blink>JavaScript
Then use javascript component, as it's related to V8 code and not it's build infrastructure.
Components: -Blink>JavaScript Blink>JavaScript>Language
Components: -Blink>JavaScript>Language Blink>JavaScript>API
Owner: yangguo@chromium.org
Status: Assigned (was: Untriaged)
Yang, any pointers?
Cc: peria@chromium.org
Owner: haraken@chromium.org
Kentaro, I think this belongs into your domain?

Comment 10 by peria@chromium.org, Apr 12 2018

Cc: -peria@chromium.org haraken@chromium.org yukishiino@chromium.org
Components: -Blink>JavaScript>API Blink>Bindings
Owner: peria@chromium.org
Yes, this is a Bindings' issue.
IDL definition looks valid, so let me check how the behavior got wrong.
Blockedon: 839389
I doubt that we should treat many issues Blockedon: the bindings generator's refactoring plan.  Can we use a hotlist or something else?
Project Member

Comment 13 by bugdroid1@chromium.org, May 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df79808dd2a54aae8307371933e7d04b05777cfa

commit df79808dd2a54aae8307371933e7d04b05777cfa
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Mon May 14 15:15:35 2018

bindings: Update overload resolution algorithm in bindings

Before this CL, overload resolution algorithm was based on old
version, and had some Blink specific customizes.
This CL updates it to be almost compatible with current spec,
except for types that Blink does not support.


Bug:  828401 
Change-Id: Ia2ee07a08f45390f39258268608fd71fdd06da89
Reviewed-on: https://chromium-review.googlesource.com/1013798
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558311}
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/scripts/v8_interface.py
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/templates/interface.cpp.tmpl
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface.cc
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_constructor_2.cc
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_origin_trial_enabled.cc
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/tests/results/core/v8_test_object.cc
[modify] https://crrev.com/df79808dd2a54aae8307371933e7d04b05777cfa/third_party/blink/renderer/bindings/tests/results/modules/v8_test_interface_partial.cc

Comment 14 by peria@chromium.org, May 15 2018

Cc: hbos@chromium.org
Status: Fixed (was: Assigned)

Comment 15 by hbos@chromium.org, May 15 2018

Cool, thanks. Next time there's getStats() work or refactoring to be done I'll try this out.

Comment 16 by peria@chromium.org, May 25 2018

Labels: Hotlist-Bindings-IDLCompiler

Sign in to add a comment