Closed
Bug 839106
Opened 11 years ago
Closed 11 years ago
Add preferences to control the stun behavior
Categories
(Core :: WebRTC: Networking, defect, P2)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: florian, Assigned: florian)
Details
(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])
Attachments
(1 file, 1 obsolete file)
2.45 KB,
patch
|
jesup
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•11 years ago
|
||
https://hg.mozilla.org/projects/alder/rev/ec82038ad4e1
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
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. :-)
Updated•11 years ago
|
Priority: -- → P2
Whiteboard: [WebRTC], [blocking-webrtc+]
Comment 6•11 years ago
|
||
On re-reading I agree with derf.
Updated•11 years ago
|
Assignee: nobody → jib
Comment 7•11 years ago
|
||
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-
Assignee | ||
Comment 8•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #714368 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf5f56832b4
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 10•11 years ago
|
||
I landed the name change on alder too: https://hg.mozilla.org/projects/alder/rev/9ef36f0c93ef
Updated•11 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
Comment 11•11 years ago
|
||
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.
Description
•