Preparation for new Promise-based RTCPeerConnection.getStats. (issue 2156063002 by hbos@chromium.org)

[hb...@chromium.org]

Reviewers: Mike West, hta - Chromium, tommi-chrömium
CL: https://codereview.chromium.org/2156063002/

Message:
Please take a look, mkwst and hta. (And tommi when back from vacation.)

Description:
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. 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-statsPromise.html.

Due to WebIDL limitations, callback interfaces and interfaces are
indistinguishable[2] when deciding which getStats function to call. This CL gets
around this by defining getStats(optional any value) in WebIDL and examining the
value's type information in C++. See crbug.com/629068.

[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
[2] https://heycam.github.io/webidl/#idl-overloading

BUG=627816, 629068

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+201, -179 lines):
A third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html
A third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise-expected.txt
M third_party/WebKit/Source/modules/modules.gypi
A + third_party/WebKit/Source/modules/peerconnection/RTCLegacyStatsReport.h
A + third_party/WebKit/Source/modules/peerconnection/RTCLegacyStatsReport.cpp
A + third_party/WebKit/Source/modules/peerconnection/RTCLegacyStatsReport.idl
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
D third_party/WebKit/Source/modules/peerconnection/RTCStatsReport.h
D third_party/WebKit/Source/modules/peerconnection/RTCStatsReport.cpp
D third_party/WebKit/Source/modules/peerconnection/RTCStatsReport.idl
M third_party/WebKit/Source/modules/peerconnection/RTCStatsResponse.h
M third_party/WebKit/Source/modules/peerconnection/RTCStatsResponse.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCStatsResponse.idl
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

[h...@chromium.org]

Good to see this code!
Reviewed from the back of an IETF meeting, so don't assume I've caught
everything.
High points:

- Verify your assumptions on IDL overloading rules
- Use testharness.js

I like the part about doing the move-the-old-stuff-out-of-the-way as a largely
separate operation!



https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html#newcode4
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:4:
<script src="../../resources/js-test.js"></script>
This test is new. Can you make it a testharness.js-based test instead of
js-test.js?

I think the guidance is that all new tests should be
testharness.js-based (which makes it possible to upstream them to the
W3C test framework).

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html#newcode15
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:15:
navigator.webkitGetUserMedia(dictionary, callback, error);
Use navigator.mediaDevices.getUserMedia => promise for all new stuff,
even in tests.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html#newcode44
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:44:
shouldNotThrow('pc.getStats().then(onResolve, onReject2)');
In general, when testing promises, it's better to write it as

pc.getStats()
.then(args => {
stuff
})
.catch(error => {
stuff
});

This saves you from defining a million oddly named helper functions.

there's a style guide on formatting these, which looks odd, but appears
to be the least unreasonable one.
See the comments in the getConstraints tests, which uses this format.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode387
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:387:
// than one argument it is invoked with the subsequent arguments
undefined.
Is this just a description of how v8::Function works, or is there
something special about this particular usage of v8::Function?

If it's just general, I suggest removing it.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode1004
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1004:
resolver->reject();
Where do we set the error to be returned in this case?

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl#newcode116
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116:
// dispatch to the correct |getStats| function based on the value type.
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?

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl#newcode124
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:124:
// evaluate the value type ourselves in C++. crbug.com/629068, 627816.
Can you add UMA counters here while you're at it?

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

PTAL hta.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html#newcode4
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:4:
<script src="../../resources/js-test.js"></script>

On 2016/07/19 09:56:48, hta - Chromium wrote:
> This test is new. Can you make it a testharness.js-based test instead
of
> js-test.js?
>
> I think the guidance is that all new tests should be
testharness.js-based (which
> makes it possible to upstream them to the W3C test framework).

Done. Much nicer.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html#newcode15
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:15:
navigator.webkitGetUserMedia(dictionary, callback, error);

On 2016/07/19 09:56:48, hta - Chromium wrote:
> Use navigator.mediaDevices.getUserMedia => promise for all new stuff,
even in
> tests.

Done.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html#newcode44
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:44:
shouldNotThrow('pc.getStats().then(onResolve, onReject2)');

On 2016/07/19 09:56:48, hta - Chromium wrote:
> In general, when testing promises, it's better to write it as
>
> pc.getStats()
> .then(args => {
> stuff
> })
> .catch(error => {
> stuff
> });
>
> This saves you from defining a million oddly named helper functions.
>
> there's a style guide on formatting these, which looks odd, but
appears to be
> the least unreasonable one.
> See the comments in the getConstraints tests, which uses this format.

Done.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode387
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:387:
// than one argument it is invoked with the subsequent arguments
undefined.

On 2016/07/19 09:56:48, hta - Chromium wrote:
> Is this just a description of how v8::Function works, or is there
something
> special about this particular usage of v8::Function?
>
> If it's just general, I suggest removing it.

Done.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode1004
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1004:
resolver->reject();

On 2016/07/19 09:56:48, hta - Chromium wrote:
> Where do we set the error to be returned in this case?

This error was undefined. I added an error and updated other rejection
code to use rejectWithDOMException instead.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl#newcode116
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116:
// dispatch to the correct |getStats| function based on the value type.

On 2016/07/19 09:56:48, hta - Chromium wrote:
> 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?

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.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl#newcode124
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:124:
// evaluate the value type ourselves in C++. crbug.com/629068, 627816.

On 2016/07/19 09:56:48, hta - Chromium wrote:
> Can you add UMA counters here while you're at it?

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).

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

PTAL hta.

I updataed the RTCPeerConnection.idl comments to be agnostic as to why it is
unable to distinguish between the two signatures.

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl#newcode116
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116:
// dispatch to the correct |getStats| function based on the value type.

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?

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

+asvitkine for UserCounter.h and histograms.xml (UseCounter::count /w
UseCounter::RTCPeerConnectionGetStats called from RTCPeerConnection.cpp:1025).

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

And +bokan also for UserCounter.h and histograms.xml (UseCounter::count /w

UseCounter::RTCPeerConnectionGetStats called from RTCPeerConnection.cpp:1025).

(asvitkine: histograms.xml owner, bokan: UseCounter.h owner)

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl#newcode124
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:124:
// evaluate the value type ourselves in C++. crbug.com/629068, 627816.

On 2016/07/19 13:59:35, hbos_chromium wrote:
> On 2016/07/19 09:56:48, hta - Chromium wrote:

> > Can you add UMA counters here while you're at it?
>
> 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).

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.

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

Oops, +bokan wasn't added. There.

https://codereview.chromium.org/2156063002/

[bo...@chromium.org]

UseCounter.h and usage lgtm

https://codereview.chromium.org/2156063002/

[asvi...@chromium.org]

lgtm



https://codereview.chromium.org/2156063002/

[har...@chromium.org]

I might want to see a conclusion of the discussion on bug 629068
(https://bugs.chromium.org/p/chromium/issues/detail?id=629068#c9) before rushing
to land the hand-coded overload resolution algorithm.


https://codereview.chromium.org/2156063002/

[h...@chromium.org]

Agree that we should wait a day or two to see if Haraken's folks can manage to
sort out the code generator before submitting this code. Wanted to get a few
more comments on test formatting in - functional code seems OK with me.


https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
(right):

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html#newcode10
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:10:
var pc = new webkitRTCPeerConnection(null);
I don't think you should indent the Javascript due to the HTML tags. It
wastes horizontal space.

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html#newcode13
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:13:
return navigator.mediaDevices.getUserMedia({audio:true,
video:true}).then(function(mediaStream) {
Note that the examples in
https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style
break .then on a new line. I think this is more readable.

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html#newcode33
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:33:
assert_unreached('Expected promise to be rejected.');
Use form "The promise should be rejected".

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

On 2016/07/21 23:02:37, hta - Chromium wrote:
> Agree that we should wait a day or two to see if Haraken's folks can manage to
> sort out the code generator before submitting this code. Wanted to get a few
> more comments on test formatting in - functional code seems OK with me.

I agreed. They've already put a CL out. It made it possible to distinguish
between callback interface as a function and the overload, but not callback
interface as an interface, status at crbug.com/629068.

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
(right):

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html#newcode10
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:10:
var pc = new webkitRTCPeerConnection(null);

On 2016/07/21 23:02:37, hta - Chromium wrote:
> I don't think you should indent the Javascript due to the HTML tags.
It wastes
> horizontal space.

Other RTCPeerConnection testharness tests look this way, but I agree
with you, removing indentation.

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html#newcode13
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:13:
return navigator.mediaDevices.getUserMedia({audio:true,
video:true}).then(function(mediaStream) {

On 2016/07/21 23:02:37, hta - Chromium wrote:
> Note that the examples in
>
https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style
> break .then on a new line. I think this is more readable.

Done.

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html#newcode33
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:33:
assert_unreached('Expected promise to be rejected.');

On 2016/07/21 23:02:37, hta - Chromium wrote:
> Use form "The promise should be rejected".

Done.

https://codereview.chromium.org/2156063002/

[to...@chromium.org]

lgtm




https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode970
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:970:
return getStats(scriptState, static_cast<MediaStreamTrack*>(nullptr));
you shouldn't need to cast a nullptr

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

PTAL hta, mkwst (and haraken?)

With the overload bug fixed (https://codereview.chromium.org/2175463002) I was
able to remove all custom overload algorithm code and use the correct .idl.

https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode970
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:970:
return getStats(scriptState, static_cast<MediaStreamTrack*>(nullptr));

On 2016/07/25 08:31:17, tommi-chrömium wrote:
> you shouldn't need to cast a nullptr

(Removed all of this code now that getStats overloading works with
WebIDL.)

https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

Bump mkwst

https://codereview.chromium.org/2156063002/

[mk...@chromium.org]

LGTM. Much nicer without the custom code, thanks!

https://codereview.chromium.org/2156063002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2156063002/240001


https://codereview.chromium.org/2156063002/

[hb...@chromium.org]

hta is OOO. I have the LGTMs, landing.

https://codereview.chromium.org/2156063002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #11 (id:240001)

https://codereview.chromium.org/2156063002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 11 (id:??) landed as
https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699
Cr-Commit-Position: refs/heads/master@{#408096}

https://codereview.chromium.org/2156063002/

[h...@chromium.org]

lgtm

post-submit lgtm.


https://codereview.chromium.org/2156063002/