Closed Bug 810458 Opened 8 years ago Closed 8 years ago

mozRTCSessionDescriptor constructor arguments don't match spec

Categories

(Core :: WebRTC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: ted, Assigned: padenot)

References

()

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-])

Attachments

(1 file, 2 obsolete files)

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
Priority: -- → P2
Whiteboard: [WebRTC][blocking-webrtc+]
Assignee: nobody → paul
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
Attachment #683474 - Flags: review?(rjesup)
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)
Attachment #683474 - Flags: review?(rjesup) → review+
Yeah, I overlooked the spec apparently. Fixed.

Carrying forward r+.
Attachment #683474 - Attachment is obsolete: true
Attachment #683542 - Flags: review+
And backed out because I forgot to think before addressing the review.

http://hg.mozilla.org/integration/mozilla-inbound/rev/fb1af8ca910e
This is the proper way to do it. Asking for rewiew again to avoid having to back
myself out in shame in 10 minutes.
Attachment #683553 - Flags: review?(rjesup)
Attachment #683542 - Attachment is obsolete: true
Attachment #683553 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/0dc53785ef12
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.