1435774 - Update ICE stat names

Status: RESOLVED DUPLICATE of bug 1324788

(Core :: WebRTC: Signaling, enhancement, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

A number of the fields in ICE related stats have names that have changed as the spec has evolved.  We should present the new names, and eventually deprecated the old ones.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1435774 - P1 - update field names in ICE stats webidl

Review commit: https://reviewboard.mozilla.org/r/217960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217960/

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1435774 - P2 - rename ICE stats fields in cpp

Review commit: https://reviewboard.mozilla.org/r/217962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217962/

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1435774 - P3 - add deprecation warnings for old ICE field names

Review commit: https://reviewboard.mozilla.org/r/217964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217964/

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948543 [details]
Bug 1435774 - P1 - update field names in ICE stats webidl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217960/diff/1-2/

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948544 [details]
Bug 1435774 - P2 - rename ICE stats fields in cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217962/diff/1-2/

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217964/diff/1-2/

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf587a8988b30153cc4d84c5307145fe6df53126

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87c60fb381a041d80a4b5aae4a0c59a93926faca

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3b899b837fa9c5a3a9c1c6796c8c2b4f6f8cef3

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Here is a manual test that can be run to verify the new warnings https://jsfiddle.net/m4fzw64v/ which are not observable from JS. The JS observable results should be covered by mochitests.

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1435774 - P4 - Update stats mochitest

Review commit: https://reviewboard.mozilla.org/r/246246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246246/

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1435774 - P5 - WebRTC stats JSON stringify mochitest

Review commit: https://reviewboard.mozilla.org/r/246248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246248/

Comment 13

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1435774 - P6 - Update about:webrtc to use new field names

Review commit: https://reviewboard.mozilla.org/r/246250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246250/

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948543 [details]
Bug 1435774 - P1 - update field names in ICE stats webidl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217960/diff/2-3/

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948544 [details]
Bug 1435774 - P2 - rename ICE stats fields in cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217962/diff/2-3/

Comment 16

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217964/diff/2-3/

Comment 17

[Michael Froman [:mjf]]

Comment on attachment 8948543 [details]
Bug 1435774 - P1 - update field names in ICE stats webidl

https://reviewboard.mozilla.org/r/217960/#review252668

Looks good to me.

Comment 18

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35481345032a22ddb06166a04a276b1330558d11

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948543 [details]
Bug 1435774 - P1 - update field names in ICE stats webidl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217960/diff/3-4/

Comment 20

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948544 [details]
Bug 1435774 - P2 - rename ICE stats fields in cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217962/diff/3-4/

Comment 21

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217964/diff/3-4/

Comment 22

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8980087 [details]
Bug 1435774 - P4 - Update stats mochitest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246246/diff/1-2/

Comment 23

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8980088 [details]
Bug 1435774 - P5 - WebRTC stats JSON stringify mochitest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246248/diff/1-2/

Comment 24

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8980089 [details]
Bug 1435774 - P6 - Update about:webrtc to use new field names

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246250/diff/1-2/

Comment 25

[Michael Froman [:mjf]]

Comment on attachment 8948544 [details]
Bug 1435774 - P2 - rename ICE stats fields in cpp

https://reviewboard.mozilla.org/r/217962/#review253032

Looks good to me.

Comment 26

[Michael Froman [:mjf]]

Comment on attachment 8980087 [details]
Bug 1435774 - P4 - Update stats mochitest

https://reviewboard.mozilla.org/r/246246/#review253034

Looks good to me.

Comment 27

[Michael Froman [:mjf]]

Comment on attachment 8980088 [details]
Bug 1435774 - P5 - WebRTC stats JSON stringify mochitest

https://reviewboard.mozilla.org/r/246248/#review253050

Looks good to me.

Comment 28

[Michael Froman [:mjf]]

Comment on attachment 8980089 [details]
Bug 1435774 - P6 - Update about:webrtc to use new field names

https://reviewboard.mozilla.org/r/246250/#review253056

Looks good to me.

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

https://reviewboard.mozilla.org/r/217964/#review253864

Please split out isRemote and remote-* type changes into separate patch as discussed offline.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:236
(Diff revision 4)
>        // roundTripTime
>        if (stat.inner.isRemote) {
>          ok(stat.roundTripTime >= 0, stat.type + ".roundTripTime is sane with" +
>            "value of:" + stat.roundTripTime);
>        } else {
> +        dump(`@@NG localVideoOnly ${JSON.stringify(stat.inner)} isRemote = ${stat.inner.isRemote}\n`);

remove debug cruft

Comment 30

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

https://reviewboard.mozilla.org/r/217964/#review253866

::: dom/media/PeerConnection.js:416
(Diff revision 4)
>          "candidate-pair": "candidatepair",
>          "local-candidate": "localcandidate",
>          "remote-candidate": "remotecandidate"
> +  },
> +  // TODO improve message
> +  _isRemoteWarning:

Missed refactoring out isRemoteWarning here.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:17
(Diff revision 4)
>    });
>  var statsExpectedByType = {
>    "inbound-rtp": {
> -    expected: ["id", "timestamp", "type", "ssrc", "isRemote", "mediaType",
> +    expected: ["id", "timestamp", "type", "ssrc", "mediaType",
>        "packetsReceived", "packetsLost", "bytesReceived", "jitter",],
> -    optional: ["roundTripTime", "remoteId", "nackCount",],
> +    optional: ["isRemote", "roundTripTime", "remoteId", "nackCount",],

Missed backing out these changes. To isRemote here.

Comment 31

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

https://reviewboard.mozilla.org/r/217964/#review253868

::: dom/media/tests/mochitest/test_peerConnection_stats.html:30
(Diff revision 4)
>      deprecated: ["mozRtt"],
>    },
>    "outbound-rtp": {
> -    expected: ["id", "timestamp", "type", "ssrc", "isRemote", "mediaType",
> +    expected: ["id", "timestamp", "type", "ssrc", "mediaType",
>        "packetsSent", "bytesSent", "remoteId",],
> -    optional: ["remoteId", "nackCount",],
> +    optional: ["isRemote", "remoteId", "nackCount",],

Missed backing out these changes to isRemote here.

Comment 32

[Nils Ohlmeier [:drno]]

Nico are these patches still on your radar? Can you merge them now after libwebrtc 64 landed?

Comment 33

[Nico Grunbaum [:ng, @chew:mozilla.org]]

I landed the stat name changes in bug 1324788, so I am going to mark this as a dupe.