Closed Bug 1100508 Opened 10 years ago Closed 9 years ago

No easy way to capture data for a bug report from about:webrtc

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: jesup, Assigned: pkerr)

References

Details

(Whiteboard: [reporting])

Attachments

(1 file, 11 obsolete files)

Easily capture about:webrtc page data for a bug reports
65.69 KB, patch
pkerr
: review+
Details | Diff | Splinter Review 
To report info from about:webrtc, you have to manually copy and paste data, and the formatting gets badly munged (especially the ICE info).  And you have to do it for 3 tabs and then maybe hit connection log and save that.

A button that copies all the info for a peerconnection (or just everything) to a debug file and pops a file-save dialog would be a huge win.  While much easier to read and use, it's at times harder to get users to report data with the updates.
(In reply to Randell Jesup [:jesup] from comment #0)
> A button that copies all the info for a peerconnection (or just everything)
> to a debug file and pops a file-save dialog would be a huge win.

Or maybe instead of a file, output to a text field for easy pasting to the clipboard?
Depends on: 1100502, 1100505
Hi Paul -- This is the main bug we want to fix this quarter to enable a better reporting/debugging tool for the field.
Assignee: nobody → pkerr
Paul, please see the discussion in bug 1129748. If you're planning to use React.js for the updated about:webrtc page, please consider moving that library to toolkit too.
I'm happy discuss things, you know where to find me ;)
Blocks: 1129748
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Paul, please see the discussion in bug 1129748. If you're planning to use
> React.js for the updated about:webrtc page, please consider moving that
> library to toolkit too.
> I'm happy discuss things, you know where to find me ;)

If this discussion does happen, can it please be recorded here so that people not directly involved can read it over and review it? Thanks in advance!

Granted that I have no clout in the matter, but I have some thoughts on this:
* Why should React be added to toolkit? Granted it isn't huge, but there's been a large push recently to slim down Firefox as much as possible, this seems to *directly* counter that. (Additionally, why should other apps need to package another few hundred KB just to display an about page. That's quite wasteful.)
* React seems to directly make it harder for contributors to contribute:
  * It makes it harder to figure out what the code is trying to do.
  * It is more difficult to casually develop on this resource: it requires a developer to learn yet another library & makes it harder to actually get any changes in (you need to install *another* dependency to convert this jsx thing to js, which isn't even part of the build system).
* This pushes Firefox code away from established standards, which is something Mozilla tries to actively promote, and toward a proprietary library controlled by a third-party.
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Paul, please see the discussion in bug 1129748. If you're planning to use
> React.js for the updated about:webrtc page, please consider moving that
> library to toolkit too.

Please strongly consider dropping the dependency on React. It would require anyone using the webrtc API who wishes to glance at the code that produces about:webrtc (to better understand what it is doing and what the output means, or to improve it, or indeed to review changes to it), to learn some React -- especially as 'View source' will only show you the preprocessed JS, and not the JSX that produced it. While React may be useful for Loop, using it in toolkit for an about page seems unnecessary.
Mike, aleth, Patrick: I have noted your comments and will add more working comments to this bug, and maintain coordination with 1129748, as I progress. I do share some of your concerns about (no pun intended) the current page.
(In reply to aleth [:aleth] from comment #5)
> Please strongly consider dropping the dependency on React. It would require
> anyone using the webrtc API who wishes to glance at the code that produces
> about:webrtc (to better understand what it is doing and what the output
> means, or to improve it, or indeed to review changes to it), to learn some
> React

React has great documentation: http://facebook.github.io/react/docs/getting-started.html. We're likely to experiment with it more in Firefox code, not less, and so it probably wouldn't hurt to brush up on it regardless.

Any new tool makes it "harder for contributors to contribute", assuming "contributors" are a static resource that can't adapt. But that's a silly way to frame things: new tools also let you do new things. No one would argue that we should stop adding features to the web (like canvas, or WebSockets, or flexbox) because they make it "harder for contributors to contribute". This is really very similar, except that the primary motivation for this pushback is a general distaste about the concept of using libraries at all, and/or NIH syndrome, and I do not share that viewpoint.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #7)
> Any new tool makes it "harder for contributors to contribute", assuming
> "contributors" are a static resource that can't adapt. But that's a silly
> way to frame things: new tools also let you do new things. No one would
> argue that we should stop adding features to the web (like canvas, or
> WebSockets, or flexbox) because they make it "harder for contributors to
> contribute". This is really very similar, except that the primary motivation
> for this pushback is a general distaste about the concept of using libraries
> at all, and/or NIH syndrome, and I do not share that viewpoint.

Presumed motives aside, it is possible to consider React an interesting library and yet disagree with this particular use case, which is what I tried to express with my comment. Of course I'm well aware I may not have the full picture.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #7)

> the primary motivation
> for this pushback is a general distaste about the concept of using libraries
> at all, and/or NIH syndrome, and I do not share that viewpoint.

I'm surprised you know what other people's "primary motivation" is.

Here are the concerns I have with the way React has been used in about:webrtc:

- preprocessed/compiled (or "transpiled") files are checked-in, and when stumbling on them from mxr, it's not obvious that these files are not worth reading directly.

- hacking the code requires installing additional tools. If we seriously want to use React more, jsx should be included in mozilla-build, and should be in the set of tools installed when following the "get started" instructions on MDN.

- the way the react library is currently included is very fragile, especially with the version number in the file name. This has already caused bustage, see bug 1118255.

- including a version of react in toolkit/ will enable add-on authors to depend on it, which apparently means they'll depend on this specific version of the library. That means each update of the library in the future will be a pain.

- the source code of that library isn't fully in the tree currently. http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/browser/components/loop/content/shared/libs/react-0.12.2.js#l4 is unreadable minified code. This issue has already been raised in the past (bug 1022871).

- I haven't seen the intent of using this library (or just to 'experiment' with it) announced anywhere. If we are going to use it in toolkit (or even just in random places of browser/), I think reviewers for these code areas need to know it, and take some time to get familiar with it. I would expect to see this posted (and discussed!) at least to firefox-dev and dev-platform (where new library dependencies are usually announced).

- having more and more minified libraries in the tree makes using tools like mxr increasingly difficult. Eg. last Friday while digging into bug 1124678, here is what MXR returned to me: http://i.imgur.com/YxeP3jJ.png (arguably tools could be improved to ignore files that don't need to be indexed).

- how is this code going to be maintained? It seems facebook is behind this library. Do we know if they'll still maintain it a few years from now? Do we have any experience contributing patches to them? If we start depending on it, we'll need it to be maintained for a long time.

I think this last point is the only one that could be attributed at least in part to the "NIH syndrome". What about the others?
I should be posting some work-in-progress patches this week that allow for the page data to be "flattened" in order to be captured in a file. I am evaluating the use of React given the simple, mostly static, nature of this about page. The React library has proven to be very useful for the Hello client, based on my experience and others. I will be evaluating whether use of React will provide the interested user an easier path to providing WebRTC logs when a problem is encountered, and the trade-offs of including the library.
I am not tied to use of React for about:webrtc, it may very well make sense not to use it there - I'll defer entirely to Paul's judgement.

The specific issues you raise Florian are valid, but not all should be treated as blockers to expanding use of this library, and all can be addressed with some work.
Working demo of one-button data capture to file (with a simplified page layout - not the final version) can be found in the patch set on:

http://hg.mozilla.org/users/paulrkerr_gmail.com/bug1100508_aboutWebRTC/

I need to work on switching between the data-hiding fold/tab view in the original and the flattened presentation needed for saving to a file.
WIP for functional feedback
Page with simplified structure. The full page will be expanded when using either the "Save This Page" control or selecting to print the page from the browser menu. File locations are also displayed when the debug log is enabled or AEC capture is enabled.

This is for function testing; final formatting and UX work to be completed.
Attachment #8568047 - Flags: feedback?(rjesup)
Attachment #8568047 - Flags: feedback?(mreavy)
Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [reporting]
small style update for control section
Attachment #8568047 - Attachment is obsolete: true
Attachment #8568047 - Flags: feedback?(rjesup)
Attachment #8568047 - Flags: feedback?(mreavy)
Attachment #8568753 - Flags: feedback?(rjesup)
Attachment #8568753 - Flags: feedback?(mreavy)
Attachment #8568753 - Attachment is obsolete: true
Attachment #8568753 - Flags: feedback?(rjesup)
Attachment #8568753 - Flags: feedback?(mreavy)
Attachment #8568919 - Attachment is obsolete: true
Re-worked page rendering. Additional styling to control section.
Attachment #8569523 - Attachment is obsolete: true
Tested on OS X 10.9.5 and Win7 with e10s disabled. On OS X with e10s, only half of a call looped back to the same browser will have data displayed. A fix for this issue is part of bug 1100502.
whitespace purge
Attachment #8572171 - Attachment is obsolete: true
Comment on attachment 8572233 [details] [diff] [review]
Capture all about:webrtc data for a bug report

Initial platform review. Will also get a browser review.
Attachment #8572233 - Flags: review?(jib2007)
Attachment #8572233 - Flags: review?(jib2007)
Comment on attachment 8572233 [details] [diff] [review]
Capture all about:webrtc data for a bug report

Platform review.
Attachment #8572233 - Flags: review?(jib)
Comment on attachment 8572233 [details] [diff] [review]
Capture all about:webrtc data for a bug report

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

Looks good! Just a bunch of style nits. No hablo CSS though, so someone else needs to look at that.

r=me with nits.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +17,5 @@
> +                                   "@mozilla.org/filepicker;1", "nsIFilePicker");
> +
> +const _ = document;
> +
> +let getReports = new Promise((resolve, reject) => {

Nit: no need to define reject when it is unused. Smaller, simpler, faster. Applies throughout.

On naming: getReports sounds like the function itself rather than the result of a function. Promises represent the result, so I worry this is confusing.

How about haveReports, or at least past-tense gottenReports? That seems to fit the general "SomeFutureState.then()" naming pattern. e.g. loaded.then(), opened.then() etc. that I've seen.

@@ +20,5 @@
> +
> +let getReports = new Promise((resolve, reject) => {
> +  WebrtcGlobalInformation.getAllStats((stats) => {
> +    resolve(stats);
> +  });

Embrace the arrow: WebrtcGlobalInformation.getAllStats(stats => resolve(stats));

Applies throughout.

@@ +26,5 @@
> +
> +let getLog = new Promise((resolve, reject) => {
> +  WebrtcGlobalInformation.getLogging('', (log) => {
> +    resolve(log);
> +  });

Why don't WebrtcGlobalInformation.getStats() and WebrtcGlobalInformation.getLogging() return promises? Maybe in a follow-up?

@@ +36,5 @@
> +    let set = ControlSet.render();
> +    ControlSet.add(new SavePage());
> +    ControlSet.add(new DebugMode());
> +    ControlSet.add(new AecLogging());
> +    controls.appendChild(set);

These three buttons appear in a vertical column on the far left, with nothing to its right, taking up a lot of vertical space. Why not across?

@@ +44,5 @@
> +  if (!content) {
> +    return;
> +  }
> +
> +  Promise.all([getReports, getLog]).then(

Here's where the naming is confusing I think. It reads like this code invokes two functions here, rather than reacting to results of invocations that have happened prior. Maybe it's just me.

@@ +45,5 @@
> +    return;
> +  }
> +
> +  Promise.all([getReports, getLog]).then(
> +     ([stats, log]) => {

That's cool. Never thought of that. :)

@@ +49,5 @@
> +     ([stats, log]) => {
> +       AboutWebRTC.init(stats.reports, log);
> +       content.appendChild(AboutWebRTC.render());
> +     },
> +    (error) => {

I would do .then(a).catch(b) here instead of .then(a,b), otherwise if AboutWebRTC.init() fails for any reason, even a software bug, then the error is silenced.

@@ +51,5 @@
> +       content.appendChild(AboutWebRTC.render());
> +     },
> +    (error) => {
> +      let msg = _.createElement("h3");
> +      msg.textContent = "ERROR: Cannot retrieve WebRTC log data";

Instead of "ERROR" maybe error.name?

Also, why not output the actual error.message as well?

If not, what's the error strategy on this page? Could we emit the full error to web console, a'la http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?rev=b7e05aa7a82b#543

@@ +92,5 @@
> +    let controlElem = _.createElement("button");
> +    let messageElem = _.createElement("span");
> +
> +    this.ctrl = controlElem;
> +    controlElem.onclick = this.onClick.bind(this);

Or: controlElem.onclick = () => this.onClick();

@@ +122,5 @@
> +  },
> +
> +  onClick: function(event) {
> +    return true;
> +  },

What does this do?

@@ +155,5 @@
> +
> +    let nodes = content.querySelectorAll(".no-print");
> +    let noPrintList = [];
> +    for (let node of nodes) {
> +      noPrintList.push({

This is an Array.map.

@@ +281,5 @@
> +      if (a.timestamp < b.timestamp) {
> +        return -1;
> +      }
> +      if (a.timestamp > b.timestamp) {
> +        return 1;

reports.sort((a, b) => a.timestamp - b.timestamp);

@@ +310,5 @@
> +      div, {showMsg: "show log", hideMsg: "hide log"});
> +    sectionCtrl.appendChild(foldEffect.render());
> +    content.appendChild(sectionCtrl);
> +
> +    this.log.forEach((line) => {

Parenthesis unnecessary on single arg arrow. Applies throughout.

@@ +344,5 @@
> +    div.appendChild(iceStats.render());
> +    let sdpStats = new SDPStats(this.report);
> +    div.appendChild(sdpStats.render());
> +    let rtpStats = new RTPStats(this.report);
> +    div.appendChild(rtpStats.render());

Maybe easier to read if they were inlined?

@@ +355,5 @@
> +    let pcInfo = this.getPCInfo(this.report);
> +    let heading = _.createElement("h3");
> +    let now = new Date(this.report.timestamp);
> +    heading.textContent =
> +      `[ ${pcInfo.id} ] ${pcInfo.url} ${pcInfo.closed ? "(closed)" : ""} ${now.toTimeString()}`;

Maybe put .toTimeString() on now above, then this would fit under 80 chars (you do that elsewhere).

@@ +517,5 @@
>      if (stats.packetsReceived) {
>        statsString += ` Received: ${stats.packetsReceived} packets`;
>  
>        if (stats.bytesReceived) {
> +        statsString += ` (${this.round00(stats.bytesReceived/1024)} Kb)`;

Use (stats.bytesReceived/1024).toFixed(2) instead.

@@ +576,5 @@
> +IceStats.prototype = {
> +  render: function() {
> +    let tbody = [];
> +    this.generateIceStats().forEach((stat) => {
> +      tbody.push([

Use Array.map.

@@ +592,5 @@
> +       "Priority", "Nominated", "Selected"],
> +      tbody);
> +
> +    let div = _.createElement("div"),
> +        heading = _.createElement("h4");

I'd go for single line or individual lets.

@@ +607,5 @@
> +    // iceCandidateStats array.
> +    let candidates = new Map();
> +
> +    this.report.iceCandidateStats.forEach((c) => {
> +      candidates.set(c.id, c);

Fits on one line.

@@ +650,5 @@
> +    });
> +
> +    stats.sort((a, b) => {
> +      return b.priority - a.priority;
> +    });

stats.sort((a, b) => b.priority - a.priority);
Attachment #8572233 - Flags: review?(jib) → review+
When doing a remote call rather than a local-loop, I don't get any output in about:webrtc at all in e10s mode: http://jsfiddle.net/nnc13tw2 - I guess that's known? Hopefully we can fix this soon.
I also see that the saved path is output to the right of the Save Page button, so I guess the space isn't entirely unused, but it still seems a bit wasted. If the Save Page button came last, then maybe it would still work to have them across? My save path was fairly short on OSX, maybe not so on Windows?
Comment on attachment 8572233 [details] [diff] [review]
Capture all about:webrtc data for a bug report

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

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +36,5 @@
> +    let set = ControlSet.render();
> +    ControlSet.add(new SavePage());
> +    ControlSet.add(new DebugMode());
> +    ControlSet.add(new AecLogging());
> +    controls.appendChild(set);

> These three buttons appear in a vertical column on the far left, with nothing to its right, taking up a lot of vertical space. Why not across?

Agreed.

Also, if it still looks like it did the last time I tried it, they look like links, and not buttons.  They should look like buttons as they don't take you to a new page.
(In reply to Randell Jesup [:jesup] from comment #26)
> Comment on attachment 8572233 [details] [diff] [review]
> Capture all about:webrtc data for a bug report
> 
> Review of attachment 8572233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/aboutwebrtc/aboutWebrtc.js
> @@ +36,5 @@
> > +    let set = ControlSet.render();
> > +    ControlSet.add(new SavePage());
> > +    ControlSet.add(new DebugMode());
> > +    ControlSet.add(new AecLogging());
> > +    controls.appendChild(set);
> 
> > These three buttons appear in a vertical column on the far left, with nothing to its right, taking up a lot of vertical space. Why not across?
> 
> Agreed.
> 
> Also, if it still looks like it did the last time I tried it, they look like
> links, and not buttons.  They should look like buttons as they don't take
> you to a new page.

They have been changed back to buttons in this patch and no longer appear a link text. The reason for keeping them to the left side is to allow for a caption to be rendered to the right of each button to show the file name and location used by the operation. For example, after you click the "Start Debug Mode" button it will display "logging debug messages to: /tmp/WebRTC.log" and the button now displays "Stop Debug Mode".

I will give some thought to how to better show that each button has an associated status caption.
Yes see comment 25. But does each button need its own status area, or could they reuse the same area?

Oh and Please don't make the area more pronounced on my account, I like minimalist. :)
incorporate review feedback
Attachment #8572233 - Attachment is obsolete: true
Jesup: What succinct instructions would you post for guide a user that turned on AEC capture? I am thinking of a sentence like: "have your caller speak for n seconds while you are silent".
Flags: needinfo?(rjesup)
Attachment #8577001 - Attachment is obsolete: true
Changes:

Added a "Save Page" control that allows the about:webrtc page to written to a file. After the file is saved, a message is generated to remind the user where the file was saved.

Added screen and print only css that will correctly expand the page data and hide the controls if the user chooses to use the Print function of the browser instead of the "Save Page" button.

Added status messages to the Debug Mode and AEC Logging buttons to indicate when those features are active and to list where the webrtc log file and AEC sample files are stored after capture. Hopefully listing where the files are dropped will help the user follow through and attach them to bugs or email.

Dropped tabbed sections used for each set of PeerConnection data to allow for the direct expansion of collapsed sections in place when saving the about:webrtc page to a file. (This also allows for the cut-and-paste of all PeerConnection data in one operation.)

Dropped the dynamic table sorting controls from ICE Statistics table to simplify the page.

Removed the usage of the React library. Since this is a page of static information, I felt that a simpler approach was better.
Comment on attachment 8578184 [details] [diff] [review]
Capture all about:webrtc data for a bug report

Browser peer review.
Attachment #8578184 - Flags: review?(florian)
Attachment #8578184 - Flags: review?(florian)
Comment on attachment 8578184 [details] [diff] [review]
Capture all about:webrtc data for a bug report

Platform peer review completed. Asking for a browser peer review. Ping me if you have any questions.
Attachment #8578184 - Flags: review?(mdeboer)
Hi Paul, apologies for the delay, I'll review this tomorrow morning! (my time)
Comment on attachment 8578184 [details] [diff] [review]
Capture all about:webrtc data for a bug report

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

I stopped reviewing aboutWebrtc.js at about 2/3, because I've gathered enough general feedback that can be applied throughout the code to allow for a much improved version next round.

Overall I like the simplifications and the removal of the React stuff. Vanilla JS FTW! :)

::: toolkit/content/aboutwebrtc/aboutWebrtc.css
@@ +5,5 @@
>  
> +.controls {
> +    font-size: 1.1em;
> +    display: inline-block;
> +    margin: 0em 0.5em;

nit: just `0` should be enough

@@ +13,5 @@
> +    margin: 0.5em 0em;
> +}
> +
> +.control > button {
> +    margin: 0em 0.25em;

nit: just `0` should be enough

@@ +36,5 @@
>      border-radius: 10px;
>      background: none repeat scroll 0% 0% #FFF;
>  }
>  
> +.peer-connection h3, .log h3 {

Please put additional selectors on its own line.
Please use sibling selectors whenever possible (.peer-connection > h3)

@@ +75,4 @@
>  }
>  
> +@media screen {
> +    .fold-closed, .fold-closed * {

When you `display: none;` only the `.fold-closed` elements, that would also hide its children, no?
In that case you can get rid of 'any' selector here.

Same goes for the selector in the `@media print` section below.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +26,5 @@
> +  WebrtcGlobalInformation.getLogging('', log => resolve(log))
> +);
> +
> +function onLoad() {
> +  let controls = _.body.querySelector("#controls");

I think `document.querySelector` would work just as well. Can you change that throughout this file?

@@ +41,5 @@
> +    return;
> +  }
> +
> +  Promise.all([reportsRetrieved, logRetrieved]).then(
> +    ([stats, log]) => {

nit: can you format this as:

```js
.then([stats, log] => {
  ...
}).catch(error => {
  ...
});
```

?

@@ +78,5 @@
> +  add: function(controlObj) {
> +    let [controlElem, messageElem] = controlObj.render();
> +    this.controlSection.appendChild(controlElem);
> +    this.messageSection.appendChild(messageElem);
> +  },

nit: trailing comma.

@@ +129,5 @@
> +  },
> +
> +  onClick: function(event) {
> +    return true;
> +  },

nit: trailing comma.

@@ +149,5 @@
> +  let content = _.body.querySelector("#content");
> +
> +  if (!content) {
> +    return;
> +  }

nit: you can omit the braces here.

@@ +175,5 @@
> +    FileUtils.closeAtomicFileOutputStream(fout);
> +
> +    noPrintList.forEach(x => {
> +      if (x.displayStyle) {
> +        x.node.style.setProperty("dislplay", x.displayStyle);

Did this ever work?

@@ +178,5 @@
> +      if (x.displayStyle) {
> +        x.node.style.setProperty("dislplay", x.displayStyle);
> +      } else {
> +        x.node.style.removeProperty("display")
> +      };

nit: please remove the semi-colon here. You may also write this without braces.

@@ +205,5 @@
> +
> +DebugMode.prototype.offStateLabel = "Start Debug Mode";
> +DebugMode.prototype.offStateMessage = "trace log can be found at: ";
> +DebugMode.prototype.onStateLabel = "Stop Debug Mode";
> +DebugMode.prototype.onStateMessage = "debug mode active, writing trace messages to: ";

Since these are really just string constants and not used outside this file, I think you can write them as constants. Even better when you put them in a separate, localizable string bundle!

@@ +213,5 @@
> +  try {
> +    let file = Services.prefs.getCharPref("media.webrtc.debug.log_file");
> +    this.message = this.onStateMessage + file;
> +  }
> +  catch (e) {

nit: please put this on the same line as the first closing brace.

@@ +260,5 @@
> +
> +AecLogging.prototype.offStateLabel = "Start AEC Logging";
> +AecLogging.prototype.offStateMessage = "captured log files can be found in: ";
> +AecLogging.prototype.onStateLabel = "Stop AEC Logging";
> +AecLogging.prototype.onStateMessage = "AEC logging active (speak with the caller for a few minutes and then stop the capture)";

Same as the comment at `DebugMode`

@@ +268,5 @@
> +  try {
> +    let file = Services.prefs.getCharPref("media.webrtc.debug.aec_log_dir");
> +    this.message = this.offStateMessage + file;
> +  }
> +  catch (e) {

nit: please put this on the same line as the first closing brace.

@@ +297,5 @@
> +};
> +
> +let AboutWebRTC = {
> +  reports: [],
> +  log: [],

Please mark these as private with an `_`

@@ +313,4 @@
>    },
>  
> +  renderPeerConnections: function() {
> +    let connections = _.createElement("div");

Perhaps you can also use a documentFragment here?

@@ +328,4 @@
>    },
>  
> +  renderConnectionLog: function() {
> +    let content = _.createElement("div");

Same as above.

@@ +453,5 @@
> +
> +RTPStats.prototype = {
> +  render: function() {
> +    let div = _.createElement("div"),
> +        heading = _.createElement("h4");

nit: please put each variable declaration on its own line.

::: toolkit/content/aboutwebrtc/aboutWebrtc.xhtml
@@ +8,5 @@
>  ]>
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>    <head>
>      <title>Webrtc Internals</title>

When will we make this page localizable? I see strings hardcoded throughout the JS file as well, which should be in a .properties bundle.

Is this something that you can do in this bug or in a follow-up right after?

@@ +9,5 @@
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>    <head>
>      <title>Webrtc Internals</title>
> +    <meta charset="UTF-8" />

I don't think this meta tag is necessary
Attachment #8578184 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #36)
> Comment on attachment 8578184 [details] [diff] [review]
> Capture all about:webrtc data for a bug report
> 
> Review of attachment 8578184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When will we make this page localizable? I see strings hardcoded throughout
> the JS file as well, which should be in a .properties bundle.
> 
> Is this something that you can do in this bug or in a follow-up right after?
> 
I have discussed this with mreavy and there is no current plan to localize the this page. That being said, I will take some more steps with the strings used in this script to make it easier to go the L10N route in the future.
(In reply to Mike de Boer [:mikedeboer] from comment #36)
> Comment on attachment 8578184 [details] [diff] [review]
> Capture all about:webrtc data for a bug report
> 
> Review of attachment 8578184 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +41,5 @@
> > +    return;
> > +  }
> > +
> > +  Promise.all([reportsRetrieved, logRetrieved]).then(
> > +    ([stats, log]) => {
> 
> nit: can you format this as:
> 
> ```js
> .then([stats, log] => {
>   ...
> }).catch(error => {
>   ...
> });
> ```
I have to keep the parenthesis around the arrow function argument: ".then(([stats, log]) => {" or else I would get a "SyntaxError: invalid arrow-function arguments".
Surprising but true. FYI, I've raised this here out of interest:
https://mail.mozilla.org/pipermail/es-discuss/2015-March/042125.html
> 
> @@ +313,4 @@
> >    },
> >  
> > +  renderPeerConnections: function() {
> > +    let connections = _.createElement("div");
> 
> Perhaps you can also use a documentFragment here?
> 
> @@ +328,4 @@
> >    },
> >  
> > +  renderConnectionLog: function() {
> > +    let content = _.createElement("div");
> 
> Same as above.
> 
I made the suggested change for the PeerConnection blocks but chose to keep the connection log in its own div for logical separation.
> 
> @@ +175,5 @@
> > +    FileUtils.closeAtomicFileOutputStream(fout);
> > +
> > +    noPrintList.forEach(x => {
> > +      if (x.displayStyle) {
> > +        x.node.style.setProperty("dislplay", x.displayStyle);
> 
> Did this ever work?
>
I over-thought this and was doing a save and restore where one was not necessary. I simplified the .no-print handling.
no longer needed
Flags: needinfo?(rjesup)
incorporate feedback comments
Attachment #8578184 - Attachment is obsolete: true
Attachment #8580363 - Flags: review?(mdeboer)
Comment on attachment 8580363 [details] [diff] [review]
Capture all about:webrtc data for a bug report

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

Three questions:

1) I started Debug Mode and AEC capturing, MacOS fx-team with this patch applied, made a Hello call with both ends connected, ended call and stopped both modes. about:webrtc didn't render/ refresh the table after I did this. Perhaps it's a nice feature to do this?
1) How do you plan to make sure this page doesn't break in the future? In other words: can you add an integration test (I'd say mochitest-browser test)?
2) Can you file a [UX] bug and CC sevaan, psackl and mmaslaney to make this page look more awesome? I can see that we'll be getting more and more ppl using this page and making it look nice will only make that more pleasant. I realize this won't be a high prio thing, but I'd be happy to take a stab at implementing such a design!

I know I put down a lot of comments, but please don't let that put you off! This is shaping up nicely, so I'm ready for the next round :)

::: toolkit/content/aboutwebrtc/aboutWebrtc.css
@@ +1,1 @@
>  html {

Can you land this file with lowercase hex color values and corrected indentation to be 2 spaces?

@@ +35,3 @@
>      border: 1px solid #AFACA9;
>      border-radius: 10px;
>      background: none repeat scroll 0% 0% #FFF;

Can you change this to just `#fff`?

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +6,2 @@
>  
>  "use strict";

Can you move this to hug the license block comment?

@@ +6,4 @@
>  
>  "use strict";
>  
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

You're only really using `Cu`, so please remove the others.

@@ +16,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "FilePicker",
> +                                   "@mozilla.org/filepicker;1", "nsIFilePicker");
> +
> +const strings = {
> +  cannot_retrive_log: "Cannot retrieve WebRTC log data",

s/cannot_retrive_log/cannot_retrieve_log/

@@ +44,5 @@
> +  local: "local",
> +  remote: "remote",
> +  local_candidate: "Local Candidate",
> +  remote_candidate: "Remote Candidate",
> +  ice_state: "Ice State",

nit: please keep ICE usage consistent: s/Ice/ICE/

@@ +59,5 @@
> +  avg_bitrate_label: "Avg. bitrate",
> +  avg_framerate_label: "Avg. framerate",
> +  dropped_frames_label: "Dropped frames",
> +  discarded_packets_label: "Discarded packets",
> +  decoder_label: " Decoder",

Can you put the leading space in the template string instead?

@@ +65,5 @@
> +  received_label: "Received",
> +  packets: "packets",
> +  lost_label: "Lost",
> +  jitter_label: "Jitter",
> +  sent_label: "Sent",

nit: trailing comma.

@@ +69,5 @@
> +  sent_label: "Sent",
> +};
> +
> +let reportsRetrieved = new Promise(resolve =>
> +  WebrtcGlobalInformation.getAllStats(stats => resolve(stats))

nit: missing `;`

@@ +73,5 @@
> +  WebrtcGlobalInformation.getAllStats(stats => resolve(stats))
> +);
> +
> +let logRetrieved = new Promise(resolve =>
> +  WebrtcGlobalInformation.getLogging('', log => resolve(log))

s/''/""/ and end with a `;`

@@ +130,5 @@
> +  }
> +};
> +
> +function Control() {
> +  this.label = null;

nit: please prefix private members with `_`

@@ +225,5 @@
> +};
> +
> +function DebugMode() {
> +  Control.call(this);
> +  this.messageHeader = strings.debug_mode_msg_label;

nit: please prefix private members with `_`

@@ +229,5 @@
> +  this.messageHeader = strings.debug_mode_msg_label;
> +
> +  if (WebrtcGlobalInformation.debugLevel > 0) {
> +    this.onState();
> +    return;

Using a `return` in a constructor looks a bit odd to me... can you put the two statements below in an `else` block instead?

@@ +245,5 @@
> +  try {
> +    let file = Services.prefs.getCharPref("media.webrtc.debug.log_file");
> +    this.message = strings.debug_mode_on_state_msg + file;
> +  }
> +  catch (e) {

nit: please put this on the same line as the closing brace of try

@@ +274,5 @@
> +};
> +
> +function AecLogging() {
> +  Control.call(this);
> +  this.messageHeader = strings.aec_logging_msg_label;

nit: please prefix private members with `_`

@@ +278,5 @@
> +  this.messageHeader = strings.aec_logging_msg_label;
> +
> +  if (WebrtcGlobalInformation.aecDebug) {
> +    this.onState();
> +    return;

Using a `return` in a constructor looks a bit odd to me... can you put the two statements below in an `else` block instead?

@@ +294,5 @@
> +  try {
> +    let file = Services.prefs.getCharPref("media.webrtc.debug.aec_log_dir");
> +    this.message = strings.aec_logging_off_state_msg + file;
> +  }
> +  catch (e) {

nit: please put this on the same line as the closing brace of try

@@ +339,5 @@
>  
> +  renderPeerConnections: function() {
> +    let connections = document.createDocumentFragment();
> +
> +    let reports = this._reports.slice(0);

If cloning is your primary objective here, you can also do:
`let reports = [...this._reports];`

@@ +340,5 @@
> +  renderPeerConnections: function() {
> +    let connections = document.createDocumentFragment();
> +
> +    let reports = this._reports.slice(0);
> +    reports.sort((a,b) => a.timestamp - b.timestamp);

s/a,b/a, b/

@@ +341,5 @@
> +    let connections = document.createDocumentFragment();
> +
> +    let reports = this._reports.slice(0);
> +    reports.sort((a,b) => a.timestamp - b.timestamp);
> +    reports.forEach(report => {

`for (let report of reports) {`

@@ +361,5 @@
> +    let div = document.createElement("div");
> +    let sectionCtrl = document.createElement("div");
> +    sectionCtrl.className = "section-ctrl no-print";
> +    let foldEffect = new FoldEffect(
> +      div, {showMsg: strings.log_show_msg, hideMsg: strings.log_hide_msg});

nit: can you format this as:

```js
let foldEffect = new FoldEffect(div, {
  showMsg: strings.log_show_msg,
  hideMsg: strings.log_hide_msg
});
```
?

@@ +365,5 @@
> +      div, {showMsg: strings.log_show_msg, hideMsg: strings.log_hide_msg});
> +    sectionCtrl.appendChild(foldEffect.render());
> +    content.appendChild(sectionCtrl);
> +
> +    this._log.forEach(line => {

`for (let line of this._log) {`

@@ +470,5 @@
> +};
> +
> +function RTPStats(report) {
> +  this.report = report;
> +  this.stats = [];

nit: please prefix private members with `_`

@@ +498,2 @@
>  
>      rtpStats.forEach(function(stats) {

`for (let stats of rtpStats) {`

@@ +501,5 @@
>          remoteRtpStats[stats.id] = stats;
>        }
>      });
>  
>      rtpStats.forEach(function(stats) {

`for (let stats of rtpStats) {`

Can you also add a comment above this loop to tell us why we need this second loop? (I know you didn't write this ;) )

@@ +532,3 @@
>  
>      if (stats.bitrateMean) {
> +      statsString += ` ${strings.avg_bitrate_label}: ${(stats.bitrateMean/1000000).toFixed(2)} Mbps`;

nit: add spaces next to operator.

@@ +535,2 @@
>        if (stats.bitrateStdDev) {
>          statsString += ` (${(stats.bitrateStdDev/1000000).toFixed(2)} SD)`;

nit: add spaces next to operator.

@@ +558,5 @@
>  
> +    let line = document.createElement("p");
> +    line.textContent = statsString;
> +    return line;
> + },

nit: indentation

@@ +616,5 @@
> +  },
> +};
> +
> +function IceStats(report) {
> +  this.report = report;

nit: please prefix private members with `_`

@@ +653,5 @@
> +    // Create an index based on candidate ID for each element in the
> +    // iceCandidateStats array.
> +    let candidates = new Map();
> +
> +    this.report.iceCandidateStats.forEach(c => candidates.set(c.id, c));

`for (let candidate of this.report.iceCandidateStats) {`

@@ +663,5 @@
> +    let stat;
> +    let local;
> +    let remote;
> +
> +    this.report.iceCandidatePairStats.forEach(pair => {

`for (let pair of this.report.iceCandidatePairStats) {`

@@ +664,5 @@
> +    let local;
> +    let remote;
> +
> +    this.report.iceCandidatePairStats.forEach(pair => {
> +      local = candidates.get(pair.localCandidateId);

You can simply define the scope here as `let local = ` and `let remote = `.

@@ +668,5 @@
> +      local = candidates.get(pair.localCandidateId);
> +      remote = candidates.get(pair.remoteCandidateId);
> +
> +      if (local) {
> +        stat = {

I vote for just doing `let stat = `... we have block scopes, might as well use 'em as such.

@@ +694,5 @@
> +      }
> +    });
> +
> +    stats.sort((a, b) => (b.priority || 0) - (a.priority || 0));
> +    return stats;

You can shorten this to `return stats.sort(...)`, as it's an in-place operation.

@@ +710,5 @@
> +};
> +
> +function SimpleTable(heading, data) {
> +  this.heading = heading || [];
> +  this.data = data;

nit: please prefix private members with `_`

@@ +718,5 @@
> +  renderRow: function(list) {
> +    let row = document.createElement("tr");
> +    let cell;
> +
> +    list.forEach(elem => {

`for (let elem of list) {`

@@ +719,5 @@
> +    let row = document.createElement("tr");
> +    let cell;
> +
> +    list.forEach(elem => {
> +      cell = document.createElement("td");

nit: `let cell = ...`

@@ +737,2 @@
>  
> +    this.data.forEach(row => {

`for (let row of this._data) {`

@@ +746,5 @@
> +function FoldEffect(targetElem, options = {}) {
> +  if (!targetElem)
> +    return;
> +
> +  this.showMsg = options.showMsg || strings.fold_show_msg;

Please prefix private members with `_`.

@@ +760,5 @@
>  
> +    let ctrl = document.createElement("a");
> +    this.trigger = ctrl;
> +    ctrl.className = "fold-trigger";
> +    ctrl.href = "#";

Since this doesn't behave as an anchor, can you just make it a <div> instead? Added benefit: doesn't mess with your scroll position with you click it. An href set to `#` does.

@@ +795,3 @@
>  
> +FoldEffect.expandAll = function() {
> +  this._sections.forEach(section => {

`for (let section of this._sections) {`

@@ +801,3 @@
>  
> +FoldEffect.collapseAll = function() {
> +  this._sections.forEach(section => {

`for (let section of this._sections) {`
Attachment #8580363 - Flags: review?(mdeboer) → review-
Maire, what's the reason for not wanting to localize this page? Do we intend to uplift this patch once it's done?
If not, I don't really understand what we're saving in terms of time or resources by hard-coding the strings in here by not putting them in a .properties bundle.
Status: NEW → ASSIGNED
(In reply to Mike de Boer [:mikedeboer] from comment #44)
> Comment on attachment 8580363 [details] [diff] [review]
> Capture all about:webrtc data for a bug report
> 
> Review of attachment 8580363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Three questions:
> 
> 1) I started Debug Mode and AEC capturing, MacOS fx-team with this patch
> applied, made a Hello call with both ends connected, ended call and stopped
> both modes. about:webrtc didn't render/ refresh the table after I did this.
> Perhaps it's a nice feature to do this?

The underlying WebrtcGlobalInformation module that is being used by this about page does not have a mechanism to notify when the set of PeerConnections has changed. This page has, to this point, relied on the user doing a refresh. I am currently doing an overhaul of WebrtcGlobalInformation to fix e10s issues. I can open a bug to add such a feature to WebrtcGlobalInformation and implement it in a later version of the about:webrtc page. I would tackle this after the e10s fix, which is more critical at this point.

> 1) How do you plan to make sure this page doesn't break in the future? In
> other words: can you add an integration test (I'd say mochitest-browser
> test)?

I would suggest a bug to create an integration test as there is interest in getting the page improvements for making the capture and collection of this data out in this cycle. An integration test is important but can be a follow-on.

> 2) Can you file a [UX] bug and CC sevaan, psackl and mmaslaney to make this
> page look more awesome? I can see that we'll be getting more and more ppl
> using this page and making it look nice will only make that more pleasant. I
> realize this won't be a high prio thing, but I'd be happy to take a stab at
> implementing such a design!
>

Will do. There has been discussion among the platform team about adding help information and more stuff about webrtc for the interested early adopter. The priority may be lower, as you say, than getting this bit of functionality out but there is not reason to stop at this patch.
now with a en-US property file
Attachment #8580363 - Attachment is obsolete: true
Attachment #8581008 - Flags: review?(mdeboer)
Comment on attachment 8581008 [details] [diff] [review]
Capture all about:webrtc page data for a bug reports

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

I couldn't see any major issues - thanks!!

r=me with comments addressed.

::: toolkit/content/aboutwebrtc/aboutWebrtc.css
@@ +1,1 @@
>  html {

Please add a license block here.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +15,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "FilePicker",
> +                                   "@mozilla.org/filepicker;1", "nsIFilePicker");
> +
> +const strings = Services.strings.createBundle(
> +  "chrome://global/locale/aboutWebrtc.properties");

Please make this:

```js
XPCOMUtils.defineLazyGetter(this, "bundle", () => {
  Services.strings.createBundle("chrome://global/locale/aboutWebrtc.properties");
});
```

And I think it's OK to use the more verbose `bundle.GetStringFromName()` and `bundle.formatStringFromName()` instead of the shorthands you defined below.

@@ +123,5 @@
> +    this.ctrl.textContent = this._label;
> +
> +    if (this._message) {
> +      this.msg.innerHTML =
> +        `<span class='info-label'>${this._messageHeader}:</span> ${this._message}`;

nit: double quotes, please.

@@ +217,5 @@
> +  if (WebrtcGlobalInformation.debugLevel > 0) {
> +    WebrtcGlobalInformation.debugLevel = 0;
> +    this.offState();
> +  } else {
> +    WebrtcGlobalInformation.debugLevel = 65535;

Perhaps cool to add a comment explaining this large integer (even though I know, I'm not sure if other do too)

@@ +234,5 @@
> +  } else {
> +    this._label = getString("aec_logging_off_state_label");
> +    this._message = null;
> +  }
> +};

nit: unnecessary semicolon.

@@ +325,5 @@
> +    };
> +
> +    content.appendChild(div);
> +    return content;
> +  },

nit: unnecessary comma.

@@ +385,5 @@
>        id: report.pcid.match(/id=(\S+)/)[1],
>        url: report.pcid.match(/url=([^)]+)/)[1],
>        closed: report.closed
>      };
>    },

nit: unnecessary comma.

@@ +391,4 @@
>  
> +function SDPStats(report) {
> +  this._report = report;
> +};

nit: unnecessary semicolon.

@@ +423,5 @@
>  
> +function RTPStats(report) {
> +  this._report = report;
> +  this._stats = [];
> +};

nit: unnecessary semicolon.

@@ +569,5 @@
> +  },
> +
> +  labelize: function(label) {
> +    return `${label.charAt(0).toUpperCase()}${label.slice(1)}`;
> +  },

nit: unnecessary comma.

@@ +592,5 @@
> +    };
> +
> +    let statsTable = new SimpleTable(
> +      [getString("local_candidate"), getString("remote_candidate"), getString("ice_state"),
> +       getString("priority"), getString("nominated"), getString("selected")],

With the more verbose GetStringFromName you can perhaps you Array-comprehensions to sweeten things up ;) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Array_comprehensions

@@ +642,5 @@
> +        stats.push(stat);
> +      }
> +    };
> +
> +    candidates.forEach(c => {

`for (let c of candidates.values()) {`

@@ +643,5 @@
> +      }
> +    };
> +
> +    candidates.forEach(c => {
> +      if (!matched[c.id]) {

nit:

```js
if (matched[c.id])
  continue;
```

@@ +661,5 @@
> +      type = `${c.candidateType}-${c.mozLocalTransport}`;
> +    }
> +
> +    return `${c.ipAddress}:${c.portNumber}/${c.transport}(${type})`;
> +  },

nit: unnecessary comma.

@@ +667,5 @@
> +
> +function SimpleTable(heading, data) {
> +  this._heading = heading || [];
> +  this._data = data;
> +};

nit: unnecessary semicolon.

@@ +694,5 @@
> +      table.appendChild(this.renderRow(row));
> +    };
> +
> +    return table;
> +  },

nit: unnecessary comma.

@@ +708,2 @@
>    }
> +};

nit: unnecessary semicolon.

@@ +744,1 @@
>    },

nit: unnecessary comma.

::: toolkit/locales/en-US/chrome/global/aboutWebrtc.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +

nit: superfluous newline.

@@ +5,5 @@
> +
> +cannot_retieve_log = Cannot retrieve WebRTC log data
> +save_page_label = Save Page
> +# Note to translators:
> +# - %1$S will be replaced by saved page file name.

I see you took aboutTelemetry.properties as an example, which is entirely reasonable, but I think it's better to take aboutSupport.properties as a template for these comments.
Attachment #8581008 - Flags: review?(mdeboer) → review+
Attachment #8581008 - Attachment is obsolete: true
Comment on attachment 8581760 [details] [diff] [review]
Easily capture about:webrtc page data for a bug reports

Carry forward r=jib and r=mikedeboer.
Attachment #8581760 - Flags: review+
changed document.title to localizable string
Attachment #8581760 - Attachment is obsolete: true
Comment on attachment 8581862 [details] [diff] [review]
Easily capture about:webrtc page data for a bug reports

r=jib and r=mikedeboer
Attachment #8581862 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6f9fee1b03d7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Thanks!
http://hg.mozilla.org/mozilla-central/diff/6f9fee1b03d7/toolkit/locales/en-US/chrome/global/aboutWebrtc.properties#l1.66

> sent_label = Sent"
I assume the final double quote should not be there?

Nit: localization notes should look like this, bug I realize this file it's not the only one in that folder doing its own variation on the theme
http://hg.mozilla.org/mozilla-central/file/6f9fee1b03d7/toolkit/locales/en-US/chrome/global/aboutSupport.properties#l5

Is there a page somewhere explaining what all these acronyms mean (AEC, SDP, ICE, etc.)? If there is one, it would be nice to put a link in the .properties file to get a better idea to localizers.
Flags: needinfo?(pkerr)
Francesco:
You are correct, the final double quote is a mistake.
I can incorporate more information as found in the aboutSupport.properties file. I assume I should file a bug with which to attach a follow on aboutWebrtc.properties file.
Flags: needinfo?(pkerr)
(In reply to Paul Kerr [:pkerr] from comment #57)
> I can incorporate more information as found in the aboutSupport.properties
> file. I assume I should file a bug with which to attach a follow on
> aboutWebrtc.properties file.

That would be great. We should also try to land the fix before merge day (next Monday).
Blocks: 1147153
Depends on: 1408716
No longer depends on: 1408716
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: