Status

()

defect
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Makkes, Assigned: rgauthier)

Tracking

(Blocks 1 bug)

Trunk
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Assignee: nobody → mail
Component: Untriaged → WebRTC
Product: Firefox → Core
OS: Linux → All
Hardware: x86_64 → All
Priority: -- → P3
Target Milestone: --- → mozilla33
Target Milestone: mozilla33 → mozilla35
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!
Assignee: mail → rgauthier
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #8492285 - Flags: feedback?(nperriault)
Attachment #8492285 - Flags: feedback?(docfaraday)
At a work-week, will give this a look next week.
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 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.
Attachment #8492285 - Flags: feedback?(nperriault) → feedback+
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.
Attachment #8492285 - Attachment is obsolete: true
Attachment #8492285 - Flags: feedback?(docfaraday)
Attachment #8495363 - Flags: feedback?(nperriault)
Attachment #8495363 - Flags: feedback?(docfaraday)
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.
The patch also appears to cut off the first character of many of the lines under "RTP Stats."
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 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 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 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?
Attachment #8495363 - Flags: feedback?(nperriault) → feedback+
Attachment #8495363 - Attachment is obsolete: true
Attachment #8495363 - Flags: feedback?(docfaraday)
(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.
(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 :)
(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.
Attachment #8496099 - Flags: feedback?(nperriault)
Attachment #8496099 - Flags: feedback?(docfaraday)
(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 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!
Attachment #8496099 - Flags: feedback?(docfaraday) → feedback+
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.
Attachment #8496099 - Flags: feedback?(nperriault) → feedback+
Addressed Nico's comment.
Attachment #8496099 - Attachment is obsolete: true
Attachment #8496881 - Attachment is obsolete: true
Attachment #8497542 - Attachment is obsolete: true
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
Attachment #8497545 - Flags: review?(pkerr)
Attachment #8497545 - Flags: review?(nperriault)
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)
Attachment #8497545 - Flags: review?(nperriault) → review-
Attachment #8497545 - Attachment is obsolete: true
Attachment #8497545 - Flags: review?(pkerr)
Attachment #8504140 - Flags: review?(pkerr)
Attachment #8504140 - Flags: review?(nperriault)
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.
Attachment #8504140 - Flags: review?(nperriault) → review+
Attachment #8504140 - Flags: review?(pkerr) → review+
https://hg.mozilla.org/mozilla-central/rev/3352a380a448
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1084499
Flags: qe-verify-
Depends on: 1129748
You need to log in before you can comment on or make changes to this bug.