mozRTCSessionDescriptor constructor arguments don't match spec

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ted, Assigned: padenot)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

Updated

5 years ago
Priority: -- → P2
Whiteboard: [WebRTC][blocking-webrtc+]
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

5 years ago
Created attachment 683474 [details] [diff] [review]
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
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+
(Assignee)

Comment 3

5 years ago
Created attachment 683542 [details] [diff] [review]
Make mozRTCSessionDescriptor respect the spec.

Yeah, I overlooked the spec apparently. Fixed.

Carrying forward r+.
(Assignee)

Updated

5 years ago
Attachment #683474 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #683542 - Flags: review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d66d35f64802
(Assignee)

Comment 5

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

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

Comment 6

5 years ago
Created attachment 683553 [details] [diff] [review]
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.
Attachment #683553 - Flags: review?(rjesup)
(Assignee)

Updated

5 years ago
Attachment #683542 - Attachment is obsolete: true

Updated

5 years ago
Attachment #683553 - Flags: review?(rjesup) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc53785ef12
https://hg.mozilla.org/mozilla-central/rev/0dc53785ef12
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.