1419093 - Track RTC RTP source objects interface to dictionary spec change

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P4)

Description

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

The WebIDL for contributing and synchronization sources currently specifies them as interfaces.  At TPAC 2017 it was agreed that they would become dictionaries, see https://github.com/w3c/webrtc-pc/issues/1533.  We implemented them as dictionaries in bug 1363667, before a PR was filed. This bug exists as a reminder to make sure that our WebIDL matches after the PR is filed and lands.

Comment 1

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

RTCRtpSynchronizationSource.audioLevel is no longer nullable, and needs to be computed if the RTP ssrc-audio-level header is not present[0].

[0] https://github.com/w3c/webrtc-pc/pull/1668

Comment 2

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

Attachment: Bug 1419093 - P1 - update rtp source IDLs to spec

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

Comment 3

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

Attachment: Bug 1419093 - P2 - update rtp source impl & unit tests

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

Comment 4

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

Attachment: Bug 1419093 - P3 - update rtp source js & mochi tests

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

Comment 5

[Michael Froman [:mjf]]

Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

https://reviewboard.mozilla.org/r/208270/#review214182

Looks good to me.

Comment 6

[Michael Froman [:mjf]]

Comment on attachment 8937609 [details]
Bug 1419093 - P2 - update rtp source impl & unit tests

https://reviewboard.mozilla.org/r/208272/#review214220

r+ w/ nit on commit message

::: commit-message-941e2:2
(Diff revision 1)
> +Bug 1419093 - P2 - update rtp source impl & unit tests
> +

Add more to the commit message explaining that these changes result from making all the attributes 'required' in the webidl?

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8937610 [details]
Bug 1419093 - P3 - update rtp source js & mochi tests

https://reviewboard.mozilla.org/r/208274/#review214222

Looks good to me.

Comment 8

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

There have been additional changes in the spec as of 2018/1/11. A new patch will be incoming shortly.

Comment 9

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

Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

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

Comment 10

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

Comment on attachment 8937609 [details]
Bug 1419093 - P2 - update rtp source impl & unit tests

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

Comment 11

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

Comment on attachment 8937610 [details]
Bug 1419093 - P3 - update rtp source js & mochi tests

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

Comment 12

[Michael Froman [:mjf]]

Still looks fine to me.

Comment 13

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

Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

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

Comment 14

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

Comment on attachment 8937609 [details]
Bug 1419093 - P2 - update rtp source impl & unit tests

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

Comment 15

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

Comment on attachment 8937610 [details]
Bug 1419093 - P3 - update rtp source js & mochi tests

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

Comment 16

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

Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

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

Comment 17

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

Comment on attachment 8937609 [details]
Bug 1419093 - P2 - update rtp source impl & unit tests

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

Comment 18

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

Comment on attachment 8937610 [details]
Bug 1419093 - P3 - update rtp source js & mochi tests

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

Comment 19

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

WebIDL review information: This patch updates RTCRtpContributingSource and RTCRtpSynchronizationSource to their current definitions in the webrtc-pc spec
All WebIDL are changes in patch 1.

Spec links:
RTCRtpContributingSource: https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcrtpcontributingsource
RTCRtpSynchronizationSource: https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcrtpsynchronizationsource

Comment 20

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

Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

https://reviewboard.mozilla.org/r/208270/#review219878

::: dom/webidl/RTCRtpSources.webidl:26
(Diff revision 4)
>  enum RTCRtpSourceEntryType {
>      "contributing",
>      "synchronization",
>  };
>  
> -dictionary RTCRtpSourceEntry {
> +dictionary RTCRtpSourceEntry : RTCRtpSynchronizationSource {

Could you perhaps add a comment that this is used internally only

Comment 21

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

Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

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

Comment 22

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

Comment on attachment 8937609 [details]
Bug 1419093 - P2 - update rtp source impl & unit tests

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

Comment 23

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

Comment on attachment 8937610 [details]
Bug 1419093 - P3 - update rtp source js & mochi tests

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9bca0ad6901
P1 - update rtp source IDLs to spec r=mjf,smaug
https://hg.mozilla.org/integration/autoland/rev/ee60c7ad1483
P2 - update rtp source impl & unit tests r=mjf
https://hg.mozilla.org/integration/autoland/rev/259d2de21779
P3 - update rtp source js & mochi tests r=mjf

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d9bca0ad6901
https://hg.mozilla.org/mozilla-central/rev/ee60c7ad1483
https://hg.mozilla.org/mozilla-central/rev/259d2de21779