971110 - Prettify about:webrtc

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Max Jonas Werner [:Makkes]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140130133743

Steps to reproduce:

Currently about:webrtc is not really readable. We should make it more pretty.

Comment 1

[Maire Reavy [:mreavy]]

TOkeshu has the time and interest to work on this bug now, and we really need to make about:webrtc more readable since Loop is getting ready to ship.  So I'm assigning this bug to tOkeshu.

If anyone is interested in contributing to work on this bug or any related bugs, please reach out to me and/or tOkeshu.  Thanks!

Comment 2

[Romain Gauthier [:tOkeshu]]

Attachment: WIP Bug 971110 Prettify about:webrtc

This is a WIP patch to prettify about:webrtc

Due to the amount of data we need to display/filter/collapse etc. I used React which is already in loop. It simplifies considerably the code.
Not everything is there but it's a start.

For now each peer connection report is sorted from most recent to oldest.
Everything is collapsed, you can expend it by clicking on the entries.

I plan to have tabs for Ice Stats, SDP and RTP Stats for each entry.
We may also want to have "real-time" updates.

I'm confident we can now iterate quickly on this page.

NOTE: I could not rebase on the current trunk, so the patch may be out of date on this side. Will try to fix that this week-end.

Comment 3

[Byron Campen [:bwc]]

At a work-week, will give this a look next week.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8492285 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

I'm a little confused about the duplication between aboutWebrtc.xhtml and aboutWebrtc.jsx. Is there some cleanup that needs to be done here still?

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8492285 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

Looks generally good :)

::: toolkit/content/aboutwebrtc/aboutWebrtc.jsx
@@ +20,5 @@
> +  render: function() {
> +    // Sort the reports to have the more recent at the top
> +    var reports = this.props.reports.sort(function(r1, r2) {
> +      return r2.timestamp - r1.timestamp;
> +    });

Be careful as Array#sort() mutates objects, while props should never be mutated (at least locally within a component).

@@ +99,5 @@
> +            <th>Priority</th>
> +            <th>Nominated</th>
> +            <th>Selected</th>
> +          </tr>
> +          {this.props.pairs.map(function(pair) {

second arg of the function is the index you're after.

@@ +122,5 @@
> +          <IceCandidateAddress candidate={pair.remoteCandidate} />
> +        </td>
> +        <td>{pair.state}</td>
> +        <td>{pair.mozPriority}</td>
> +        <td>{pair.nominated ? '✔' : ''}</td>

Nit: null is usually preffered to indicate nothing should be rendered.

@@ +135,5 @@
> +    var c = this.props.candidate;
> +    var type = c.candidateType;
> +
> +    if (c.type == "localcandidate" && c.candidateType == "relayed")
> +      type = c.candidateType + '-' + c.mozLocalTransport;

Nit: could be extracted in a function.

@@ +148,5 @@
> +
> +var SDP = React.createClass({
> +  render: function() {
> +    var type = this.props.type;
> +    type = type.charAt(0).toUpperCase() + type.slice(1);

Nit: could be extracted to a function.

@@ +172,5 @@
> +  logs.forEach(function(logLine){
> +    logsDiv.appendChild(document.createElement('div'))
> +           .appendChild(document.createTextNode(logLine));
> +  });
> +}

We might want to have this ported to React as well.

Comment 6

[Romain Gauthier [:tOkeshu]]

Attachment: WIP Bug 971110 Prettify about:webrtc

Addressed Nico's comments and added a README.txt file

Byron: everything is now in the aboutwebrtc subfolder. JSX files are transpiled to JavaScript so we commit the two versions.

Nico: The logs part should be ported to React indeed. However I would like to think about how we can make them more useful than the dump we have today.

I will iterate on that again to replace all strings concatenations with JavaScript string templates.

Comment 7

[Adam Roach [:abr]]

The attached patch, in the "SDP" tab renders "ocal SDP" and "emote SDP" rather than "Local SDP" and "Remote SDP". I don't know whether the string work will fix this, but please double check before landing.

Comment 8

[Adam Roach [:abr]]

The patch also appears to cut off the first character of many of the lines under "RTP Stats."

Comment 9

[Adam Roach [:abr]]

I'm not sure if it's something we can easily do in this patch, or if it should be a different patch, the the candidates in the "Ice Stats" tab really should be grouped by foundation rather than simply intermixed.

By the way, despite my nitpicking, this looks very nice. I'm really looking forward to this landing.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8495363 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

This is pretty good, but we seem to be missing the PC id, which is necessary to correlate with logging when doing a tab-to-tab call (ie; when the URLs are the same). Also, we do not seem to be marking closed PCs as closed anymore.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8495363 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

Also, while it looks like the candidate pair stats are fine, the candidate stats are gone. This is actually important as it turns out, because candidates will not necessarily be placed in a pair, but it is still important to know that they exist.

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8495363 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

Looks really good! Some ideas & suggestions you might not want to take care of too much :)

::: toolkit/content/aboutwebrtc/README.txt
@@ +13,5 @@
> +   jsx -w -x jsx . .
> +
> +jsx can also be do a one-time compile pass instead of watching if the
> +-w argument is omitted. Be sure to commit any transpiled files at the
> +same time as changes to their sources.

You should also mention that generated files should never be touched by hand, to avoid silly bitrot.

::: toolkit/content/aboutwebrtc/aboutWebrtc.jsx
@@ +151,5 @@
> +  render: function() {
> +    var report = this.props.report;
> +    var url = report.pcid.match(/http[^)]+/)[0];
> +    var pairs = this.getIceCandidatePairs(report);
> +    var i = 0;

Nit: This is not used.

@@ +166,5 @@
> +});
> +
> +var IceStats = React.createClass({
> +  sort: function(key) {
> +    this.state.pairs.sort(function(pair1, pair2) {

Nit: State shouldn't be mutated in place; var sortedPairs = this.state.pairs.slice().sort(… is better.

@@ +175,5 @@
> +      if (key === this.state.sorting)
> +        return value2.localeCompare(value1);
> +
> +      return value1.localeCompare(value2);
> +    }.bind(this));

Nit: No need to bind here.

@@ +177,5 @@
> +
> +      return value1.localeCompare(value2);
> +    }.bind(this));
> +
> +    key = (key === this.state.sorting) ? null : key;

Nit: Mutating passed arguments is discouraged practice; please create a intermediate value, or use that test inline in the following line.

@@ +194,5 @@
> +            <th>
> +              <a href="#" onClick={this.sort.bind(this, "localCandidate")}>
> +                Local candidate
> +              </a>
> +            </th>

Nit: I think you could have a local function returning the whole <tr> using a map() to get cells out of a list of labels and target properties for sorting:

sortHeadings: {
  "Local candidate": "localCandidate",
  "Remote candidate": "remoteCandidate",
  …
},

generateSortHeadings: function() {
  return Object.keys(this.sortHeadings).map(function(heading, i) {
    var sortProp = this.sortHeadings[key];
    return (
      <th>
        <a href="#" onClick={this.sort.bind(this, sortProp)}>
          {heading}
        </a>
      </th>
    );
  }.bind(this));
}

Then later <tr>{this.generateSortHeadings()}</tr>.

Also, ES6 maps and arrow functions could greatly help here. Have fun!

@@ +277,5 @@
> +
> +    rtpStats.forEach(function(stats) {
> +      if (stats.remoteId)
> +        stats.remoteRtpStats = remoteRtpStats[stats.remoteId];
> +    });

I feel like Array#reduce vould be used here :)

@@ +285,5 @@
> +
> +  dumpAvStats: function(stats) {
> +    var statsString = "";
> +    if (stats.mozAvSyncDelay) {
> +      statsString += "A/V sync: " + stats.mozAvSyncDelay + " ms ";

Nit: Template strings? I'm finding them more legible.

@@ +368,5 @@
> +            isAvStats ? this.dumpAvStats(stats) : null,
> +            this.dumpCoderStats(stats),
> +            this.dumpRtpStats(stats, "local"),
> +            remoteRtpStats ? this.dumpRtpStats(remoteRtpStats, "remote") : null
> +          ]

Would be great to have smthg like

return (
  <div>
    <h5>{stats.id}</h5>
    <AvStats stats={stats}/>
    <CoderStats stats={stats}/>
    <RtpStats stats={stats} type="local"/>
    <RtpStats stats={stats} type="remote"/>
  </div>
)

WDYT?

Comment 13

[Romain Gauthier [:tOkeshu]]

Attachment: WIP Bug 971110 Prettify about:webrtc

Comment 14

[Romain Gauthier [:tOkeshu]]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #12)
> Comment on attachment 8495363 [details] [diff] [review]
> WIP Bug 971110 Prettify about:webrtc
> 
> Review of attachment 8495363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks really good! Some ideas & suggestions you might not want to take care
> of too much :)
> 
> ::: toolkit/content/aboutwebrtc/README.txt
> @@ +13,5 @@
> > +   jsx -w -x jsx . .
> > +
> > +jsx can also be do a one-time compile pass instead of watching if the
> > +-w argument is omitted. Be sure to commit any transpiled files at the
> > +same time as changes to their sources.
> 
> You should also mention that generated files should never be touched by
> hand, to avoid silly bitrot.

Done.

> @@ +175,5 @@
> > +      if (key === this.state.sorting)
> > +        return value2.localeCompare(value1);
> > +
> > +      return value1.localeCompare(value2);
> > +    }.bind(this));
> 
> Nit: No need to bind here.

I think I need it yes, as we use this.state.sorting above.

> @@ +285,5 @@
> > +
> > +  dumpAvStats: function(stats) {
> > +    var statsString = "";
> > +    if (stats.mozAvSyncDelay) {
> > +      statsString += "A/V sync: " + stats.mozAvSyncDelay + " ms ";
> 
> Nit: Template strings? I'm finding them more legible.

I replaced all heavy concatenations by template strings.

> @@ +368,5 @@
> > +            isAvStats ? this.dumpAvStats(stats) : null,
> > +            this.dumpCoderStats(stats),
> > +            this.dumpRtpStats(stats, "local"),
> > +            remoteRtpStats ? this.dumpRtpStats(remoteRtpStats, "remote") : null
> > +          ]
> 
> Would be great to have smthg like
> 
> return (
>   <div>
>     <h5>{stats.id}</h5>
>     <AvStats stats={stats}/>
>     <CoderStats stats={stats}/>
>     <RtpStats stats={stats} type="local"/>
>     <RtpStats stats={stats} type="remote"/>
>   </div>
> )
> 
> WDYT?

Well, this part of the logs is not satisfying IMO, so I will let it like this and try to figure out if a double entry table would be better here.

I addressed the rest of the comments.

Comment 15

[Romain Gauthier [:tOkeshu]]

(In reply to Adam Roach [:abr] from comment #7)
> The attached patch, in the "SDP" tab renders "ocal SDP" and "emote SDP"
> rather than "Local SDP" and "Remote SDP". I don't know whether the string
> work will fix this, but please double check before landing.

Fixed

(In reply to Adam Roach [:abr] from comment #9)
> I'm not sure if it's something we can easily do in this patch, or if it
> should be a different patch, the the candidates in the "Ice Stats" tab
> really should be grouped by foundation rather than simply intermixed.

I don't know what is a foundation in this context. But from what I understand that could be double entry tables or merged cells. I would indeed do that in another patch after this one is landed.

> By the way, despite my nitpicking, this looks very nice. I'm really looking
> forward to this landing.

Thanks :)

Comment 16

[Romain Gauthier [:tOkeshu]]

(In reply to Byron Campen [:bwc] from comment #10)
> Comment on attachment 8495363 [details] [diff] [review]
> WIP Bug 971110 Prettify about:webrtc
> 
> Review of attachment 8495363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is pretty good, but we seem to be missing the PC id, which is necessary
> to correlate with logging when doing a tab-to-tab call (ie; when the URLs
> are the same). Also, we do not seem to be marking closed PCs as closed
> anymore.

I added the id in the header of each collapsable sections and the full PC id above the tabs.
Can you explain what is the number at the beginning of the id?

(In reply to Byron Campen [:bwc] from comment #11)
> Comment on attachment 8495363 [details] [diff] [review]
> WIP Bug 971110 Prettify about:webrtc
> 
> Review of attachment 8495363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, while it looks like the candidate pair stats are fine, the candidate
> stats are gone. This is actually important as it turns out, because
> candidates will not necessarily be placed in a pair, but it is still
> important to know that they exist.

I added them back and put them in the table with the others. The unpaired candidates are thus alone in the table. Telle me if it's useful enough that way.

Comment 17

[Byron Campen [:bwc]]

(In reply to Romain Gauthier [:tOkeshu] from comment #16)
> (In reply to Byron Campen [:bwc] from comment #10)
> > Comment on attachment 8495363 [details] [diff] [review]
> > WIP Bug 971110 Prettify about:webrtc
> > 
> > Review of attachment 8495363 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is pretty good, but we seem to be missing the PC id, which is necessary
> > to correlate with logging when doing a tab-to-tab call (ie; when the URLs
> > are the same). Also, we do not seem to be marking closed PCs as closed
> > anymore.
> 
> I added the id in the header of each collapsable sections and the full PC id
> above the tabs.
> Can you explain what is the number at the beginning of the id?
> 

   The long number is a high-resolution timestamp, the short number is a window id.

> (In reply to Byron Campen [:bwc] from comment #11)
> > Comment on attachment 8495363 [details] [diff] [review]
> > WIP Bug 971110 Prettify about:webrtc
> > 
> > Review of attachment 8495363 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Also, while it looks like the candidate pair stats are fine, the candidate
> > stats are gone. This is actually important as it turns out, because
> > candidates will not necessarily be placed in a pair, but it is still
> > important to know that they exist.
> 
> I added them back and put them in the table with the others. The unpaired
> candidates are thus alone in the table. Telle me if it's useful enough that
> way.

   It looks a little strange at first glance, but having them in the same table is actually useful when you can sort by column (makes it easy to see which candidates have been paired, and with what). I kinda like it. We may need to revisit someday when we want to display things like gathering state for the candidates, but that can come later I suppose.

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8496099 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

The result looks very good to me now. Thanks!

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8496099 [details] [diff] [review]
WIP Bug 971110 Prettify about:webrtc

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

This is super good. Nits & bikeshed, probably a few more components to create for the debug buttons and the logs. Next iteration should ask for review so we can land this baby asap :)

::: toolkit/content/aboutwebrtc/aboutWebrtc.jsx
@@ +220,5 @@
> +    "Remote candidate": "remoteCandidate",
> +    "Ice State":        "state",
> +    "Priority":         "priority",
> +    "Nominated":        "nominated",
> +    "Selected":         "selected"

Nit: if we plan further localization, we probably want to invert these key/values.

@@ +226,5 @@
> +
> +  sort: function(key) {
> +    var pairs = this.state.pairs.slice().sort(function(pair1, pair2) {
> +      var value1 = pair1[key] ? pair1[key].toString() : "";
> +      var value2 = pair2[key] ? pair2[key].toString() : "";

Nit: room for factorization.

@@ +235,5 @@
> +
> +      return value1.localeCompare(value2);
> +    }.bind(this));
> +
> +    var sorting = (key === this.state.sorting) ? null : key;

Nit: the key === this.state.sorting test could be stored in a local var as it's used twice.

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

Nit: try to keep consistent with curly braces for single line statements; I think we want them everywhere.

@@ +425,5 @@
> +         .appendChild(document.createTextNode('Logging:'));
> +  logs.forEach(function(logLine){
> +    logsDiv.appendChild(document.createElement('div'))
> +           .appendChild(document.createTextNode(logLine));
> +  });

This wants to become a component too I guess.

@@ +441,5 @@
> +
> +function setDebugButton(on) {
> +  var button = document.getElementById("debug-toggle-button");
> +  button.innerHTML = on ? "Stop debug mode" : "Start debug mode";
> +  button.onclick = on ? stopDebugMode : startDebugMode;

These also wants to become a component.

@@ +457,5 @@
> +
> +function setAECDebugButton(on) {
> +  var button = document.getElementById("aec-debug-toggle-button");
> +  button.innerHTML = on ? "Stop AEC logging" : "Start AEC logging";
> +  button.onclick = on ? stopAECDebugMode : startAECDebugMode;

These also wants to become a component.

Comment 20

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 971110 Prettify about:webrtc

Addressed Nico's comment.

Comment 21

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 971110 Prettify about:webrtc

Comment 22

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 971110 Prettify about:webrtc

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8497545 [details] [diff] [review]
Bug 971110 Prettify about:webrtc

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

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +31,5 @@
> +
> +  render: function() {
> +    var cx = React.addons.classSet;
> +    return (
> +      React.DOM.div({className: "tabs"}, 

trailing spaces in this file, but if it's generated it probably doesn't mater

Comment 24

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8497545 [details] [diff] [review]
Bug 971110 Prettify about:webrtc

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

Looks really good, but this patch currently breaks because the React js file linked is out of sync with Loop. I think we need to ensure this won't happen by mistake in the future.

::: toolkit/content/aboutwebrtc/README.txt
@@ +1,4 @@
> +Working with React JSX files
> +============================
> +
> +The about:webrtc page use [React](http://facebook.github.io/react/).

The about:webrtc page useS

::: toolkit/content/aboutwebrtc/aboutWebrtc.css
@@ +46,5 @@
> +
> +.tabs > ul {
> +  list-style-type: none;
> +  margin: 0;
> +  /* padding: 0.4em; */

Should be removed.

@@ +67,5 @@
> +  background-color: #FFF;
> +  border-left: 1px solid #AFACA9;
> +  border-top: 1px solid #AFACA9;
> +  border-right: 1px solid #AFACA9;
> +  border-bottom: none;

Nit: Maybe:

border: 1px solid #AFACA9;
border-bottom: none;

@@ +74,5 @@
> +.tabs section {
> +  clear: both;
> +}
> +
> +

Nit: One too many CR.

Also, I'd prefer we use a monospaced font for displaying the logs, if we can.

::: toolkit/content/aboutwebrtc/aboutWebrtc.xhtml
@@ +5,5 @@
> +
> +
> +<!DOCTYPE html [
> +<!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd"> %htmlDTD;
> +]>

Nit: How about switching to html5?

@@ +15,5 @@
> +  </head>
> +  <body id="body" onload="onLoad()">
> +  </body>
> +
> +  <script type="text/javascript" src="chrome://browser/content/loop/shared/libs/react-0.11.1.js"></script>

This breaks, as we recently updated React to 0.11.2 in Loop.

I'm concerned we don't have any test covering this kind of breakage, especially as it's easy forgetting about synchronizing the version number here.

Suggestions:
- Write a test (best, longer)
- Ship this page along with its own local version of React (weak, faster)

Comment 25

[Romain Gauthier [:tOkeshu]]

Attachment: Bug 971110 Prettify about:webrtc

Comment 26

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8504140 [details] [diff] [review]
Bug 971110 Prettify about:webrtc

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

Looks great. r+ conditional to a new bug filed for the missing test mentioned previously.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3352a380a448

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3352a380a448