We want to propagate most of the ICE state into about:network (or equivalent) to see what went wrong during call set up. This work is dependent on Bug 906990 being completed.

I don't if this is realistic, but I'd love to get this done by Gecko 28 (so I've set that as the target milestone). Bug 906990 isn't really useful without this bug. We're targeting Bug 906990 to be complete by Nov 15. I'm asking if this can be done by Dec 3. Gecko 28 uplifts Dec 9. If it's too tight, we should just re-target this for Gecko 29. I'm assigning this to Byron, but it may make more sense to give this to Jan-Ivar -- though Jan-Ivar's plate from Gecko 26 to Gecko 28 is pretty full already.

Ignore comment 2. It was the result of another patch having the wrong bug number in the commit message.

Byron, can you please post an initial patch so the guys in Taipei can get a sense We will probably need to port this a bit to adapt to B2G.

Byron, can you please post an initial patch so the guys in Taipei can get a sense We will probably need to port this a bit to adapt to B2G.

(In reply to Eric Rescorla (:ekr) from comment #5) > Byron, can you please post an initial patch so the guys in Taipei can get a > sense We will probably need to port this a bit to adapt to B2G. On it. Need to fiddle with patches, since jib has kinda taken ownership of one of the things in my patch queue.

About page is named about:webrtc, and is largely stolen from the about:networking page.

Also, this is my first actual JS code. So if it makes your eyes bleed, don't say I did not warn you. ;)

Compensate for changes in 902003 and 906990

Since the host field in a remote ice candidate is freeform, we should watch out for HTML injection attacks, and not use innerHTML with it. Also, remove some other uses of innerHTML, to make things more clean.

Perform an initial stats fetch on load, remove the last usage of innerHTML, and add columns for nominated and selected.

Bring up-to-date with patches landed from 902003.

Set candidate ids more correctly, and display tables of local and remote candidates on about:webrtc.

Refactoring, simplification, and moving some stuff to 906990 where it belongs.

Tab fixup, and grouping related functions.

Can I get a bit more high-level context about what this page's intended audience is, and how this is useful in general?

Others may be able to fill in here, but I believe we aim to expand this to cover more information on WebRTC related activity in the browser. Chrome has something comparable in their about:webrtc-internals page.

This is intended first to be used internally by our team, and then by other developers who understand webrtc internals, for the purpose of debugging.

Incorporate feedback from w3c mailing list, de-cruft a little, fix a bug where stats divs weren't being replaced.

Compensate for some nits on 906990

Move away from setAttribute() in favor of the more JS-flavored way, since it is more terse.

Compensate for changes on 906990.

Bring up-to-date with webidl changes in 906990

Group stats by component within each PC.

Bring up-to-date with the latest freom 906990.

Comment on attachment 8337895 [details] [diff] [review] (WIP, requires patches from 906990) Part 1. Basic about:webrtc page. If the audience is "your team" and "other developers who understand webrtc internals", I'm not sure I see why we need to ship this in Firefox. Why not just ship it in an add-on? >diff --git a/toolkit/content/aboutWebrtc.xhtml b/toolkit/content/aboutWebrtc.xhtml >+ <title>Webrtc Internals</title> If this is intended to be exposed to users, it needs to be localizable (i.e. reference an entity here, similar to how e.g. aboutSessionRestore.xhtml/dtd work). >+function getCandPairLogs(id) { >+ try { >+ var wg = new WebrtcGlobalInformation(); >+ if (wg.getCandPairLogs) { >+ wg.getCandPairLogs(id, displayLogs, console.log); >+ } else { >+ console.log("WebrtcGlobalInformation has no getCandPairLogs function"); >+ } How would this failure ("WebrtcGlobalInformation has no getCandPairLogs function") occur in practice? You shouldn't have fallback codes for situations that are impossible. Similarly further down you null check wg.getAllStats, and I don't understand when that check would fail either. >+function displayLogs(logs) { >+ var logsDiv = document.getElementById('logs'); >+ while(logsDiv.lastChild) { >+ logsDiv.removeChild(logsDiv.lastChild); >+ } Replace this with logsDiv.innerHTML = "";? >+ logsDiv.appendChild(document.createElement('h3')) >+ .appendChild(document.createTextNode('Logging:')); >+ logs.forEach(function(logLine){ >+ logsDiv.appendChild(document.createElement('div')) >+ .appendChild(document.createTextNode(logLine)); >+ }); Similarly if this ends up being too slow you'd want to use a document fragment and/or innerHTML. There's a lot of inefficient DOM manipulation in the rest of this file; no need to optimize if it doesn't pose a problem in practice, but it's worth keeping in mind. f+ because I'm still not sure I see a need to ship this, but feel free to convince me otherwise!

Gavin The use scenario here is that some user (possibly an ordinary user who is a user of some WebRTC site) experiences a call failure. We (or the site) asks them to load the panel and send us the results. The problem with an add-on is that failures can be intermittent, so it's problematic to ask them to install an add-on after it happens.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #29) > Comment on attachment 8337895 [details] [diff] [review] > (WIP, requires patches from 906990) Part 1. Basic about:webrtc page. > > If the audience is "your team" and "other developers who understand webrtc > internals", I'm not sure I see why we need to ship this in Firefox. Why not > just ship it in an add-on? > Part of the motivation was feature parity with chrome, which has a similar chrome://webrtc-internals panel built in. The convenience/cost ratio is also pretty good. If the panel were a lot heavier, I would lean towards making an extension, but this is not the case. > >diff --git a/toolkit/content/aboutWebrtc.xhtml b/toolkit/content/aboutWebrtc.xhtml > > >+ <title>Webrtc Internals</title> > > If this is intended to be exposed to users, it needs to be localizable (i.e. > reference an entity here, similar to how e.g. aboutSessionRestore.xhtml/dtd > work). I'll give that a look. > > >+function getCandPairLogs(id) { > >+ try { > >+ var wg = new WebrtcGlobalInformation(); > >+ if (wg.getCandPairLogs) { > >+ wg.getCandPairLogs(id, displayLogs, console.log); > >+ } else { > >+ console.log("WebrtcGlobalInformation has no getCandPairLogs function"); > >+ } > > How would this failure ("WebrtcGlobalInformation has no getCandPairLogs > function") occur in practice? You shouldn't have fallback codes for > situations that are impossible. Similarly further down you null check > wg.getAllStats, and I don't understand when that check would fail either. > This was useful during development for catching webidl problems; the console and JS logging were basically useless in tracking this sort of problem down. I wonder how likely it is for this to break again in the future when we muck with the webidl. > >+function displayLogs(logs) { > >+ var logsDiv = document.getElementById('logs'); > >+ while(logsDiv.lastChild) { > >+ logsDiv.removeChild(logsDiv.lastChild); > >+ } > > Replace this with logsDiv.innerHTML = "";? > I have no real expertise on the matter, but the limited reading I've done on the subject implied that innerHTML is bad mmmmkay, and that innerHTML was pretty slow for emptying a node to boot (see http://jsperf.com/innerhtml-vs-removechild/6). > >+ logsDiv.appendChild(document.createElement('h3')) > >+ .appendChild(document.createTextNode('Logging:')); > >+ logs.forEach(function(logLine){ > >+ logsDiv.appendChild(document.createElement('div')) > >+ .appendChild(document.createTextNode(logLine)); > >+ }); > > Similarly if this ends up being too slow you'd want to use a document > fragment and/or innerHTML. There's a lot of inefficient DOM manipulation in > the rest of this file; no need to optimize if it doesn't pose a problem in > practice, but it's worth keeping in mind. > It turns out that dumping thousands of log lines this way is not bad at all. I was a little worried about it too.

Regarding localization; how much do we want to do here? There is a lot of technical terminology here that only appears in an IETF specification written in english ("local candidate" might translate reasonably, but what about stuff like "ICE State"?) For sure, we could do stuff like the page title, the "Logging:" label, and the "Get/Refresh Logging" button.

Forgot the add the dtd.

Comment on attachment 8342762 [details] [diff] [review] Part 4. Add a dtd that can allow about:webrtc to be localized. Review of attachment 8342762 [details] [diff] [review]: ----------------------------------------------------------------- Is this roughly what you were asking for, dtd-wise?

Comment on attachment 8342762 [details] [diff] [review] Part 4. Add a dtd that can allow about:webrtc to be localized. This looks right, though not complete. Let's not bother for the moment given the goals of this page.

Have you had a chance to look at our responses regarding the extension/about page question?

(In reply to Byron Campen [:bwc] from comment #31) > Part of the motivation was feature parity with chrome, which has a > similar chrome://webrtc-internals panel built in. The convenience/cost ratio > is also pretty good. If the panel were a lot heavier, I would lean towards > making an extension, but this is not the case. I'm still skeptical, but I won't stand in your way. > This was useful during development for catching webidl problems; the > console and JS logging were basically useless in tracking this sort of > problem down. I wonder how likely it is for this to break again in the > future when we muck with the webidl. The console/logging shouldn't be useless for catching these problems - if you're seeing issues not covered by dependencies of bug 895548, you should file them (CC me!). The extra error handling should be removed. > I have no real expertise on the matter, but the limited reading I've done > on the subject implied that innerHTML is bad mmmmkay, and that innerHTML was > pretty slow for emptying a node to boot (see > http://jsperf.com/innerhtml-vs-removechild/6). OK, maybe my knowledge here is out of date! Like I said, don't need to worry about it until it matters in practice.

Comment on attachment 8337895 [details] [diff] [review] (WIP, requires patches from 906990) Part 1. Basic about:webrtc page. r=me on this with the error handling removed per comment 38, and the commented out code removed Other style nits: - line up dots when wrapping to a new line (e.g. appendChild calls in buildCandTableRow) - replace "while(" -> "while (" - 2 space indentation for JS

Comment on attachment 8337895 [details] [diff] [review] (WIP, requires patches from 906990) Part 1. Basic about:webrtc page. Review of attachment 8337895 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/aboutWebrtc.xhtml @@ +148,5 @@ > + > + var subDivs = {}; > + > + stats.forEach(function(stat) { > + if (!subDivs[stat.componentId]) { Here you need to check that you're getting ICE stats, since only ICE stats have componentIds. As is, this breaks with other stats, e.g. Bug 908695, and I see the word "undefined" on the page, and often the page appears blank for me (applied ontop of Bug 908695). In general, the organization seems to assume only candidate stats, which I suppose is fair. However, a slight refactor may be needed to accommodate other stats, as I'm having a bit of trouble shoving the RTP stats in here for Bug 904622, assuming we want that. If you have tips, ideas, or prefer to do this, please let me know. The organization naturally differs a bit from https://bug908695.bugzilla.mozilla.org/attachment.cgi?id=8344164

(In reply to Jan-Ivar Bruaroey [:jib] from comment #40) > Comment on attachment 8337895 [details] [diff] [review] > (WIP, requires patches from 906990) Part 1. Basic about:webrtc page. > > Review of attachment 8337895 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/aboutWebrtc.xhtml > @@ +148,5 @@ > > + > > + var subDivs = {}; > > + > > + stats.forEach(function(stat) { > > + if (!subDivs[stat.componentId]) { > > Here you need to check that you're getting ICE stats, since only ICE stats > have componentIds. As is, this breaks with other stats, e.g. Bug 908695, and > I see the word "undefined" on the page, and often the page appears blank for > me (applied ontop of Bug 908695). > > In general, the organization seems to assume only candidate stats, which I > suppose is fair. However, a slight refactor may be needed to accommodate > other stats, as I'm having a bit of trouble shoving the RTP stats in here > for Bug 904622, assuming we want that. > > If you have tips, ideas, or prefer to do this, please let me know. The > organization naturally differs a bit from > https://bug908695.bugzilla.mozilla.org/attachment.cgi?id=8344164 The RTP stats are component specific, so I would expect that if the ICE stats have a component ID, the RTP stats ought to have it too (either that, or a stream ID, and with a mapping from stream id to component id elsewhere). Or am I missing something?

In http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0093.html Harald said: > I think it's an RTP component - in Chrome, they're named "audio", "video", > "audio-rtcp" and "video-rtcp" if I remember rightly. In our code, NrIceMediaStream uses random number + hard-coded stream-name for componentId (effectively a unique stream name), which is not exactly the same. It seems we also hard-code one video and one audio stream for now: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#191 In any case, http://www.w3.org/2011/04/webrtc/wiki/Stats is clear that only RTCIceCandidatePairStats has componentId, which leads me to believe these are grouping-aids not genuine unique id's. In any case, I suspect sorting that out with the spec is going to take time, so lets see what we can land for now. If the goal for now is to separate video from audio, then I can hack that easily, since we're maxing out at one video and one audio per peerconnection. Even simpler, I may just uncomment your dump code and put some lipstick on it, just to have something. You still have a bug though: componentId is undefined for your non-pair candidate stats (as well as for my RTP stats that just landed) yet you're using componentId as an index.

I am not sure we want to expose the random number. Find me online to discuss...

(In reply to Jan-Ivar Bruaroey [:jib] from comment #42) > You still have a bug though: componentId is undefined for your non-pair > candidate stats (as well as for my RTP stats that just landed) yet you're > using componentId as an index. One of the things in 906990 is adding a componentId field to those.

That doesn't seem to match the spec. We should figure out the disconnect.

Incorporate feedback.

Unbitrot.

Unbitrot.

Unbitrot.

Forgot to remove this commented-out code.

Comment on attachment 8346907 [details] [diff] [review] (WIP, requires patches from 906990) Part 1. Basic about:webrtc page.Bug 908923: (requires patches from 906990) Part 1. Basic about:webrtc page. Review of attachment 8346907 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r+ from gavin

Filter out stats without a componentId, for now.

Fix commit message.

Comment on attachment 8347578 [details] [diff] [review] (requires patches from 906990) Part 1. Basic about:webrtc page. Review of attachment 8347578 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r+ from gavin

Comment on attachment 8346854 [details] [diff] [review] Part 4. Add a dtd that can allow about:webrtc to be localized. Review of attachment 8346854 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward feedback from :gavin, although he is ok with landing without this patch. It is trivial enough to reconstruct if need be.

Comment on attachment 8346852 [details] [diff] [review] Part 3. Disable auto-refresh in about:webrtc to cut down on jank. Review of attachment 8346852 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable. One item for your consideration; take it or leave it. ::: toolkit/content/aboutWebrtc.xhtml @@ -216,5 @@ > } > } > > -var statsDisplay = setInterval(refreshStats, 1000); > - Wouldn't it be nice to add a button that allows refreshing the stats manually without having to reload the page?

(In reply to Adam Roach [:abr] from comment #56) > Comment on attachment 8346852 [details] [diff] [review] > Part 3. Disable auto-refresh in about:webrtc to cut down on jank. > > Review of attachment 8346852 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks reasonable. One item for your consideration; take it or leave it. > > ::: toolkit/content/aboutWebrtc.xhtml > @@ -216,5 @@ > > } > > } > > > > -var statsDisplay = setInterval(refreshStats, 1000); > > - > > Wouldn't it be nice to add a button that allows refreshing the stats > manually without having to reload the page? I dunno. There's already a button (and a key combination) for that. Once we have lots of different kinds of stats to display, it might be useful to allow certain things to be refreshed in isolation.

Comment on attachment 8346846 [details] [diff] [review] Part 2. Add a button to dump the entire rlog ringbuffer to about:webrtc. Review of attachment 8346846 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable.

Comment on attachment 8346854 [details] [diff] [review] Part 4. Add a dtd that can allow about:webrtc to be localized. Marking obsolete, since we decided not to do this for now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea3277ce769 https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb5cf5a7612 https://hg.mozilla.org/integration/mozilla-inbound/rev/33a87cffeac7

https://hg.mozilla.org/mozilla-central/rev/3ea3277ce769 https://hg.mozilla.org/mozilla-central/rev/3cb5cf5a7612 https://hg.mozilla.org/mozilla-central/rev/33a87cffeac7