Just to clarify, RTCStatsCallback is a callback interface and MediaStreamTrack is an interface.

See table under https://heycam.github.io/webidl/#idl-overloading

Two interfaces are distinguishable according to table footnote a) when:

- The two identified interfaces are not the same,
- it is not possible for a single platform object to implement both interfaces, - - it is not the case that both are callback interfaces.

Why aren't the two interfaces distinguishable?

I'm not sure what "it is not possible for a single platform object to implement both interfaces" means given that there's no multiple inheritance in WebIDL and AFAIK there aren't *any* platform objects (Blink-side C++ objects) that can be wrapped as more than V8 wrapper type. Is that the bit of the footnote that you found strange as well, or were you thinking of something else?

I want to second foolip@'s confusion. I don't understand if it not being able to distinguish is by design or a bug.

Here's what I've tried. With this code...

RTCPeerConnection.idl:
  [CallWith=ScriptState] Promise<void> getStats(RTCStatsCallback successCallback, optional MediaStreamTrack? selector);
  [CallWith=ScriptState] Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector);

RTCPeerConnection.h:
  ScriptPromise getStats(ScriptState*, RTCStatsCallback* successCallback, MediaStreamTrack* selector = nullptr);
  ScriptPromise getStats(ScriptState*, MediaStreamTrack* selector = nullptr);

... it compiles, but calling getStats with an RTCStatsCallback does not work in most cases:

getStats(<obj with handleEvent func>) -> No function found that matched the signature.
getStats(<handleEvent func>) -> No function found that matched the signature.
getStats(<obj with handleEvent func>, selector) -> The callback provided as parameter 1 is not a function.
getStats(<handleEvent func>, selector) -> Works.

The MediaStreamTrack case works though: getStats(selector) and getStats().

In the generated V8RTCPeerConnection.cpp code, there is no code path that leads to the callback version of getStats for the single argument case.

Adding @haraken for comments. We seem to have hit a corner case; this should either work or fail with an error.

yukishiino: Would you help hbos@ address the issue?

There seem two issues here.

1) resolution_tests_methods() in v8_interface.py is a bit outdated, and we don't yet support callback interfaces.

2) The spec says
    callback RTCStatsCallback = void (RTCStatsReport report);
i.e. RTCStatsCallback as a callback function, however we're implementing RTCStatsCallback as a callback interface.

If we can change the definition of RTCStatsCallback to a callback function as the spec says, then the current overload resolution should work just fine.

Otherwise, we need to update the binding generator and support callback interfaces in addition to callback functions.  bashi@ is good at the overload resolution.

Thanks Yuki,

We thought about changing RTCStatsCallback to a callback function, but I believe that requires writing bindings code manually? I don't think callback functions are fully supported yet? (I ran into compile errors when I tried it.)
See https://bugs.chromium.org/p/chromium/issues/detail?id=569301#c15.

The bindings generator might need to be updated either way?

2) Side note about the spec: Our version of RTCStatsCallback and RTCStatsReport is even more outdated than the legacy version in the spec, they should be renamed to RTCLegacyStatsCallback and RTCLegacyStatsReport to avoid confusion with the spec. We're keeping them as to not break existing usages rather than to follow spec. I'm guessing most people are calling it with a callback function rather than with a callback interface, if that's true maybe we could change it. But I don't have stats to back that up.

Yeah, it seems we need to update the code generator in some ways. I'll take a look tomorrow.

Thanks.

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

commit d8a924a2dd4693f40a27b33efc2de3db46a2496e
Author: bashi <bashi@chromium.org>
Date: Fri Jul 22 06:03:54 2016

IDL: Support callback interface in overload resolution

Per the spec[1] we can treat an object as a callback interface
if an effective overload set contains the callback interface.

[1] http://heycam.github.io/webidl/#es-overloads

BUG= 629068 

Review-Url: https://codereview.chromium.org/2175463002
Cr-Commit-Position: refs/heads/master@{#407077}

[modify] https://crrev.com/d8a924a2dd4693f40a27b33efc2de3db46a2496e/third_party/WebKit/Source/bindings/scripts/v8_interface.py
[modify] https://crrev.com/d8a924a2dd4693f40a27b33efc2de3db46a2496e/third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl
[modify] https://crrev.com/d8a924a2dd4693f40a27b33efc2de3db46a2496e/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp

Thanks bashi.

I rebased with master and tried again.
Using a callback interface, it is now able to distinguish between the case where a callback function (function func(x) { ... }) is used and MediaStreamTrack?.

The case where an interface is used still fails though:
var handler = {
    handleEvent:function(x) {
        console.log('handler.handleEvent(' + x + ')');
    }
};
"Uncaught (in promise) TypeError: Failed to execute 'getStats' on 'RTCPeerConnection': The callback provided as parameter 1 is not a function."

I uploaded this in case you want to see/test https://codereview.chromium.org/2174813002.

Thanks hbos@ for reporting the issue.

(It's a shame but) our logic for interface callback is a bit broken even after the overloading bug has fixed. The code generator assumes that all callback interfaces in Blink are "single operation callback interfaces"[1] and the binding layer only accepts function objects for callback interfaces. The binding layer associates a given function with the IDL operation defined in a callback function.

This means that we doesn't implement callback interface correctly. But I guess we don't pass a non-callable object as callback interface so maybe the priority of fixing this isn't high? Ideally we should fix it but we don't have much bandwidth now :(

[1] http://heycam.github.io/webidl/#dfn-single-operation-callback-interface

I think that NodeFilter is the only callback interface in Blink that isn't a "single operation callback interface" and that should actually be a callback interface per spec. (Almost all of our callback interfaces should actually be callback functions per spec.)

I think that  issue 569301  is the main blocker for cleaning the whole mess up. If it were easy to switch between callback interface and callback function, then getStats could be switched to a callback function and the types would then be distinguishable per WebIDL.

I agree that if the code generator can generate bindings for callback functions it will be cleaner.

#14: Hmm, is there any difference between the current implementation of "callback interface" and what the spec calls "callback function"? Since only the callback function case works...

In either case, it hit me just now that this ("the callback provided as parameter 1 is not a function") is not a regression.
The overload bug has been solved, so I should be able to just overload it without any changes to RTCStatsCallback and any usage of RTCStatsCallback will continue to work as it did before.

We still have unresolved issues:
- Callback functions are not fully supported ( issue 569301 ).
- Callback interfaces are not implemented correctly.
- RTCStatsCallback should really be a callback function.

But these issues should not block me from overloading getStats.

Yeah, from JS point of view, there may not be any difference. As you said these are technically wrong in terms of spec though :(

Since the getStats signatures are not indistinguishable anymore I'll close this bug I can file new bugs for the unresolved issues.

Filed  issue 630986  and  issue 630989 .

Thanks for filing issues!

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

commit 08fa67b0f718b342b8c47725f2018c63ade8b699
Author: hbos <hbos@chromium.org>
Date: Wed Jul 27 11:40:30 2016

Preparation CL for new Promise-based RTCPeerConnection.getStats.

The WebRTC spec defines the following getStats[1]:

  partial interface RTCPeerConnection {
      Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null);
  };

  interface RTCStatsReport {
      readonly maplike<DOMString, object>;  // object: RTCStats-derived dictionaries
  };

  dictionary RTCStats {
      DOMHighResTimeStamp timestamp;
      RTCStatsType        type;
      DOMString           id;
  };

There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.

Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).

A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.

Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
  conflict for when adding the new RTCStatsReport in a follow-up CL. This should
  be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
  getStats functions. E.g. it now returns Promise<void> because the return
  values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
  "RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
  its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.

[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model

BUG= 627816 ,  629068 

Review-Url: https://codereview.chromium.org/2156063002
Cr-Commit-Position: refs/heads/master@{#408096}

[add] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/modules.gypi
[rename] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCLegacyStatsReport.cpp
[rename] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCLegacyStatsReport.h
[rename] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCLegacyStatsReport.idl
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCStatsResponse.cpp
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCStatsResponse.h
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/modules/peerconnection/RTCStatsResponse.idl
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
[modify] https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699/tools/metrics/histograms/histograms.xml