Closed Bug 839106 Opened 11 years ago Closed 11 years ago

Add preferences to control the stun behavior

Categories

(Core :: WebRTC: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: florian, Assigned: florian)

Details

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

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
We encountered today an issue with a demo that used the IP of a broken STUN server. To workaround it, we need a way to ignore the STUN server provided by the content JS code and to use instead the default STUN server.

Having this issue made us (Mark and me) think that in some demo contexts it may also be useful to tweak default STUN settings (eg if the Internet isn't reachable but there's a STUN server reachable somewhere).

I'm going to land the attached patch very soon to alder as we need it for demos. I'm not sure if we want it or part of it in mozilla-central too, so I figured I would just attach it and request review.
Attachment #711369 - Flags: review?(rjesup)
Comment on attachment 711369 [details] [diff] [review]
Patch

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

My main question is over the naming.  media.peerconnection.defaultRtcConfiguration ... I think media.peerconnection.configuration.default or media.peerconnection.stun-config.default...  I'd like some others to look at it and think about what we want for the longer term, both in which vars and what they hold.

Ekr, derf?
Attachment #711369 - Flags: review?
Attachment #711369 - Flags: feedback?(tterribe)
Attachment #711369 - Flags: feedback?(jib)
Attachment #711369 - Flags: feedback?(ekr)
Comment on attachment 711369 [details] [diff] [review]
Patch

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

I haven't reviewed the actual patch, but...

(In reply to Randell Jesup [:jesup] from comment #2)
> media.peerconnection.defaultRtcConfiguration ... I think
> media.peerconnection.configuration.default or

I'm not in love with "configuration" even though the current WebRTC draft API calls this an "RTCConfiguration". It doesn't really hint at what goes in here at all. I'd suggest media.peerconnection.default_iceservers, dropping the { "iceServers": ... } wrapper. We can create additional prefs for other fields in RTCConfiguration if that object ever grows some.

> media.peerconnection.stun-config.default...  I'd like some others to look at

stun-config is bad, too, since this can include turn: URLs as well, and that is actually a more likely thing to want to override (e.g., for enterprise deployments).

I found "media.peerconnection.ignoreCustomRtcConfig" confusing, too, because it wasn't clear what was meant by "custom"... the list provided in the pref or the list provided in the page. For fonts we call the pref "use_document_fonts", which doesn't have this problem, so I'd prefer something like media.peerconnection.use_document_iceservers
Attachment #711369 - Flags: feedback?(tterribe)
Comment on attachment 711369 [details] [diff] [review]
Patch

Since the standard uses RTCPeerConnection and RTCConfiguration, I would prefer us being consistent over accurate here. I vote for:

 - media.peerconnection.configuration.default
 - media.peerconnection.ignoreCustomConfiguration
Attachment #711369 - Flags: feedback?(jib)
To clarify, my comment is solely on naming. If we want to refactor like derf suggests, then that sounds fine also. about:config seems to use a hodge-podge of under_scores, dromedaryCase and inverseBooleans, so I don't know what the prevailing wisdom here is. :-)
Priority: -- → P2
Whiteboard: [WebRTC], [blocking-webrtc+]
On re-reading I agree with derf.
Assignee: nobody → jib
Comment on attachment 711369 [details] [diff] [review]
Patch

r-
Please rework trivially to match derf's comments, in which case r+.  (Note the { "iceservers": part.)
Attachment #711369 - Flags: review?(rjesup) → review-
Attached patch Patch v2Splinter Review
I haven't full tested this newer version of the patch (I just tested that I can establish a PeerConnection with it, and that it doesn't work if I edit the JSON of media.peerconnection.default_iceservers to make it invalid JSON).
Assignee: jib → florian
Attachment #711369 - Attachment is obsolete: true
Attachment #711369 - Flags: review?
Attachment #711369 - Flags: feedback?(ekr)
Attachment #714368 - Flags: review?(rjesup)
Attachment #714368 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf5f56832b4
OS: Mac OS X → All
Hardware: x86 → All
I landed the name change on alder too:
https://hg.mozilla.org/projects/alder/rev/9ef36f0c93ef
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
https://hg.mozilla.org/mozilla-central/rev/eaf5f56832b4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: