1335967 - discardedPackets is now packetsDiscarded in RTCInboundRTPStreamStats via spec

Status: NEW

(Core :: WebRTC, defect, P3)

Description

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

RTCInboundRTPStreamStats.discardedPackets is now packetsDiscarded in the spec [1], and we should change our implementation to match.

[1] http://rawgit.com/w3c/webrtc-stats/master/webrtc-stats.html#dom-rtcinboundrtpstreamstats-packetsdiscarded

Comment 1

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

Attachment: Bug 1335967 changing stat discardedPackets to packetsDiscarded for spec compliance

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

Comment 2

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

Comment on attachment 8832748 [details]
Bug 1335967 changing stat discardedPackets to packetsDiscarded for spec compliance

https://reviewboard.mozilla.org/r/108970/#review110354

You'll need a DOM review for the webidl change. Maybe smaug?

This is a breaking change, so it's worth mentioning in release notes. Unfortunately, I don't see a lot of options as far as deprecation warnings go. While we could have done a cycle where we instrument getters of the old attribute to warn people ahead of breaking them, this could get trigger a lot of false positives, since a lot of JS appears to copy stats wholesale for various reasons (e.g. JSON.stringify).

We should add code to adapter to shim this on older FF so people can use packetsDiscarded without having to check for both.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:588
(Diff revision 1)
>      }
>  
>      if (stats.droppedFrames) {
>        statsString += ` ${getString("dropped_frames_label")}: ${stats.droppedFrames}`;
>      }
>      if (stats.discardedPackets) {

Missed one.

Comment 3

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

Comment on attachment 8832748 [details]
Bug 1335967 changing stat discardedPackets to packetsDiscarded for spec compliance

https://reviewboard.mozilla.org/r/108970/#review110354

I am not quite ready to land this. I am working on PeerConnection.js so that discardedPackets will continue to appear with the legacy getStats(). Additionally I have a change in the localization strings of aboutWebRtc, that will I assume need peer review.

Comment 4

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

Comment on attachment 8832748 [details]
Bug 1335967 changing stat discardedPackets to packetsDiscarded for spec compliance

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

Comment 5

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

After talking with Jan-Ivar I am not altering the behavior for the legacy stats call. The strategy is to when possible keep them in sync.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8832748 [details]
Bug 1335967 changing stat discardedPackets to packetsDiscarded for spec compliance

https://reviewboard.mozilla.org/r/108970/#review114260

not happy with webrtc making these backwards incompatible changes :/
But apparently this makes the spec more consistent internally.

Comment 7

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.