Preparation for new Promise-based RTCPeerConnection.getStats.

third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl

Index: third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
diff --git a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl b/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
index 1afa9291abfb67d05ebca101393fbcccedb1385e..a3bedc27d29f8221515ec9e73efb2d0b3d7b46c8 100644
--- a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
+++ b/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
@@ -104,10 +104,26 @@ enum RTCIceConnectionState {
     // TODO(guidou): The failureCallback argument should be non-optional.
     [CallWith=ScriptState] Promise<void> setRemoteDescription(RTCSessionDescriptionInit description, VoidCallback successCallback, [Default=Undefined] optional RTCPeerConnectionErrorCallback failureCallback);
     [CallWith=ScriptState, MeasureAs=RTCPeerConnectionAddIceCandidateLegacy] Promise<void> addIceCandidate((RTCIceCandidateInit or RTCIceCandidate) candidate, VoidCallback successCallback, RTCPeerConnectionErrorCallback failureCallback);
+
     // TODO(guidou): The selector argument should the first (nullable,
     // non-optional) argument, and there should be a third failureCallback
     // argument.
-    [CallWith=ExecutionContext, LegacyInterfaceTypeChecking] void getStats(RTCStatsCallback successCallback, [Default=Undefined] optional MediaStreamTrack selector);
+    // TODO(hbos): WebIDL is not able to distinguish between
+    // |getStats(RTCStatsCallback)| and |getStats(MediaStreamTrack)| because
+    // |RTCStatsCallback| is a callback interface and |MediaStreamTrack| is an
+    // interface, see https://heycam.github.io/webidl/#idl-overloading. In order
+    // to support both signatures, |getStats(optional any)| is used. In C++ we
+    // dispatch to the correct |getStats| function based on the value type.

[hta - Chromium, 2016/07/19 09:56:48]

I read heycam to say that an interface is distinguishable from another interface
if "The two identified interfaces are not the same, it is not possible for a
single platform object to implement both interfaces, and it is not the case that
both are callback interfaces."

Does the WebIDL compiler give you an error when you try to make them separate?

[hbos_chromium, 2016/07/19 13:59:35]

I might have misinterpreted something. I'm not sure if they *should* be
distinguishable. (Could an object have a handleEvent function and still be
considered a MediaStreamTrack?)

I was able to get it to compile with both getStats, but due to the generated V8
code the wrong getStats function wound up being called, it didn't distinguish.
(In line with spec or V8 gen bug?)

I've spent a lot of time on this already. One idea was to change
RTCStatsCallback to a callback function but this isn't fully supported
(crbug.com/569301) and we would have to write a lot of V8 code manually instead.
This seemed less painful.

[hbos_chromium, 2016/07/20 09:29:48]

I've confirmed a second time (crbug.com/629068#c5) that it is unable to
distinguish. We can discuss whether this is a bug or by design there, but in the
meantime, is this an acceptable workaround?

+    //
+    // This is a workaround for the desired WebIDL which is:
+    //  [CallWith=ScriptState] Promise<void> getStats(RTCStatsCallback successCallback, optional MediaStreamTrack? selector);
+    //  [CallWith=ScriptState] Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector);
+    //
+    // The plan is to have the promise-based version replace the callback-based
+    // version and remove it, then we can use the desired WebIDL and not have to
+    // evaluate the value type ourselves in C++. crbug.com/629068, 627816.

[hta - Chromium, 2016/07/19 09:56:48]

Can you add UMA counters here while you're at it?

[hbos_chromium, 2016/07/19 13:59:35]

Good idea. I'd rather add and have that reviewed in a separate follow-up CL if
that's ok. Added a TODO comment (in RTCPeerConnection.h).

[hbos_chromium, 2016/07/20 11:28:44]

I looked into it and the change is just a couple of lines so I did it in the
same CL. It requires more owners though.

+    [CallWith=ScriptState] Promise getStats(optional any value);
+    [CallWith=ScriptState] Promise<void> getStats(RTCStatsCallback successCallback, MediaStreamTrack? selector);
 
     // https://w3c.github.io/webrtc-pc/#peer-to-peer-data-api
     // TODO(guidou): The label argument should have [TreatNullAs=EmptyString]