Preparation for new Promise-based RTCPeerConnection.getStats.

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
Committed: https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699
Cr-Commit-Position: refs/heads/master@{#408096}

[hbos_chromium, 2016-07-18 11:13:42]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 11:13:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-18 12:52:30]

Description was changed from

==========
Preparation for new Promise-based RTCPeerConnection.getStats.

BUG=
==========

to

==========
Preparation for new Promise-based RTCPeerConnection.getStats.

BUG=627816, 629068
==========

[hbos_chromium, 2016-07-18 13:01:30]

Patchset #2 (id:20001) has been deleted

[hbos_chromium, 2016-07-18 13:52:50]

Description was changed from

==========
Preparation for new Promise-based RTCPeerConnection.getStats.

BUG=627816, 629068
==========

to

==========
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
==========

[hbos_chromium, 2016-07-18 14:04:44]

hbos@chromium.org changed reviewers:
+ hta@chromium.org, mkwst@chromium.org, tommi@chromium.org

[hbos_chromium, 2016-07-18 14:04:44]

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

[hbos_chromium, 2016-07-18 14:06:51]

Description was changed from

==========
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
==========

to

==========
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-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
==========

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

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/Layo...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo...
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/Layo...
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/Layo...
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/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour...
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/Sour...
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/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour...
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/Sour...
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?

[hbos_chromium, 2016-07-19 13:48:42]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 13:49:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

PTAL hta.

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

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo...
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/Layo...
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/Layo...
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/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour...
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/Sour...
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/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour...
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/Sour...
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).

[commit-bot: I haz the power, 2016-07-19 15:13:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 15:13:35]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2016-07-20 09:23:41]

Description was changed from

==========
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-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
==========

to

==========
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-statsPromise.html.

When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.

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

BUG=627816, 629068
==========

[hbos_chromium, 2016-07-20 09:25:26]

Description was changed from

==========
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-statsPromise.html.

When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.

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

BUG=627816, 629068
==========

to

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

When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.

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

BUG=627816, 629068
==========

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

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/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116: //
dispatch to the correct |getStats| function based on the value type.
On 2016/07/19 13:59:35, hbos_chromium wrote:
> 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.

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?

[hbos_chromium, 2016-07-20 11:15:27]

hbos@chromium.org changed reviewers:
+ asvitkine@chromium.org

[hbos_chromium, 2016-07-20 11:15:27]

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

[hbos_chromium, 2016-07-20 11:26:09]

Patchset #6 (id:120001) has been deleted

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

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/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour...
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.

[hbos_chromium, 2016-07-20 11:30:04]

hbos@chromium.org changed reviewers:
+ bokan@chromium.org

[hbos_chromium, 2016-07-20 11:30:05]

Oops, +bokan wasn't added. There.

[bokan, 2016-07-20 12:02:00]

UseCounter.h and usage lgtm

[Alexei Svitkine (slow), 2016-07-20 17:44:39]

lgtm

[haraken, 2016-07-20 17:55:24]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-07-20 17:55:24]

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.

[hta - Chromium, 2016-07-21 23:02:37]

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/Lay...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
(right):

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay...
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/Lay...
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/Lay...
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".

[hbos_chromium, 2016-07-22 15:08:20]

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/Lay...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html
(right):

https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay...
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/Lay...
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/Lay...
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.

[tommi (sloooow) - chröme, 2016-07-25 08:31:18]

lgtm

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

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

[hbos_chromium, 2016-07-25 09:28:22]

Description was changed from

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

When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.

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

BUG=627816, 629068
==========

to

==========
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
==========

[hbos_chromium, 2016-07-25 09:37:19]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[hbos_chromium, 2016-07-25 09:37:43]

The CQ bit was unchecked by hbos@chromium.org

[commit-bot: I haz the power, 2016-07-25 09:37:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-25 09:37:53]

The CQ bit was unchecked by hbos@chromium.org

[hbos_chromium, 2016-07-25 09:56:22]

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/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Sou...
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.)

[hbos_chromium, 2016-07-26 11:50:53]

Bump mkwst

[Mike West, 2016-07-27 08:42:35]

LGTM. Much nicer without the custom code, thanks!

[hbos_chromium, 2016-07-27 10:17:09]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2016-07-27 10:17:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
asvitkine@chromium.org, tommi@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2156063002/#ps240001
(title: "Rebase with master")

[commit-bot: I haz the power, 2016-07-27 10:17:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-27 10:18:44]

hta is OOO. I have the LGTMs, landing.

[commit-bot: I haz the power, 2016-07-27 11:40:50]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-07-27 11:40:52]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-07-27 11:42:33]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699
Cr-Commit-Position: refs/heads/master@{#408096}
==========

[commit-bot: I haz the power, 2016-07-27 11:42:34]

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

[hta - Chromium, 2016-08-01 08:32:26]

lgtm

post-submit lgtm.