810458 - mozRTCSessionDescriptor constructor arguments don't match spec

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[(not currently active) Ted Mielczarek]

The spec says "The RTCSessionDescription() constructor takes an optional dictionary argument, descriptionInitDict, whose content is used to initialize the new RTCSessionDescription object.", but mozRTCSessionDescription expects (type,sdp) arguments:
http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#87

Comment 1

[Paul Adenot (:padenot)]

Attachment: Turn the two arguments into a dict, and set the properties to null if the keys are not present.

Jesup, looking at the hg log, you seem to be an appropriate reviewer for this kind of patch, but I am new to this project and could be wrong. Feel free to forward if you are too busy/not an appropriate person.

This simply use a js object instead of the two arguments, as the spec changed (see comment 0).

The spec talks about serialization (right before [1]), and I guess our implementation is still valid, granted the serialization pattern "{ attribute }" means we serialized in a "key : value" fashion using all the attributes of the object.

There has been a mention of updating tests, but I could not find them in m-c. Maybe it is in another branch (alder?) or in some html files somewhere (maybe anant's people page, in which case it seems that those are no longer available, or have been moved).

[1]:http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCSessionDescription

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 683474 [details] [diff] [review]
Turn the two arguments into a dict, and set the properties to null if the keys are not present.

Review of attachment 683474 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming this handles a missing dictionary correctly per the spec (I didn't check, but I think it's optional).

::: dom/media/PeerConnection.js
@@ +134,5 @@
>        throw new Error("Constructor already called");
>      }
>      this._win = win;
> +    this.type = descriptionInitDict.type || null;
> +    this.sdp = descriptionInitDict.sdp || null;

Do these handle "new RTCSessionDescription;"?  (I.e. the dictionary object is (I believe) supposed to be optional)

Comment 3

[Paul Adenot (:padenot)]

Attachment: Make mozRTCSessionDescriptor respect the spec.

Yeah, I overlooked the spec apparently. Fixed.

Carrying forward r+.

Comment 4

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d66d35f64802

Comment 5

[Paul Adenot (:padenot)]

And backed out because I forgot to think before addressing the review.

http://hg.mozilla.org/integration/mozilla-inbound/rev/fb1af8ca910e

Comment 6

[Paul Adenot (:padenot)]

Attachment: Make mozRTCSessionDescriptor respect the spec.

This is the proper way to do it. Asking for rewiew again to avoid having to back
myself out in shame in 10 minutes.

Comment 7

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc53785ef12

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0dc53785ef12