Nils Ohlmeier [:drno] Reporter
	Description • 7 years ago

Currently we show the ICE candidates which nICEr reports up. But since with trickle remote candidates get passed through JS API calls any way it should be easy to add all received remote candidates to about:webrtc.

We could either simply list the received remote candidates, or highlight them in some way in the ICE connection table.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8935950 [details]
Bug 1414169 - pt 2 - aboutwebrtc.js SimpleTable improvements.

https://reviewboard.mozilla.org/r/206796/#review212608

LGTM with nit.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:786
(Diff revision 1)
>  }
>  
>  SimpleTable.prototype = {
> -  renderRow(list) {
> +  renderRow(list, header) {
>      let row = document.createElement("tr");
> +    let elemType = (header?"th":"td");

Formatting, spaces between operators and opperands.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8935951 [details]
Bug 1414169 - pt 3 - RTCStatsReport.webidl to support about:webrtc changes for showing trickled candidates.

https://reviewboard.mozilla.org/r/206798/#review212610

Seems to be ChromeOnly stuff, r+

	Code Review Bot [:reviewbot]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8935954 [details]
Bug 1414169 - pt 6 - Add all raw candidates table (local and remote).

https://reviewboard.mozilla.org/r/206804/#review212612


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3352
(Diff revision 1)
>        query->report->mLocalSdp.Construct(
>            NS_ConvertASCIItoUTF16(localDescription.c_str()));
>        query->report->mRemoteSdp.Construct(
>            NS_ConvertASCIItoUTF16(remoteDescription.c_str()));
>        query->report->mOfferer.Construct(mJsepSession->IsOfferer());
> +      for (auto candidate : mRawTrickledCandidates) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

      for (auto candidate : mRawTrickledCandidates) {
                ^
           const  &

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8935951 [details]
Bug 1414169 - pt 3 - RTCStatsReport.webidl to support about:webrtc changes for showing trickled candidates.

https://reviewboard.mozilla.org/r/206798/#review212614

LGTM

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8935952 [details]
Bug 1414169 - pt 4 - Trickled ICE candidates are highlighted with a light blue background.

https://reviewboard.mozilla.org/r/206800/#review212616

LGTM with nits, and 1 minor thing that either needs a seperate bug filed or can be fixed here.

::: dom/media/webrtc/WebrtcGlobal.h:108
(Diff revision 1)
>          !ReadParam(aMsg, aIter, &(aResult->mIceRollbacks)) ||
>          !ReadParam(aMsg, aIter, &(aResult->mTransportStats)) ||
>          !ReadParam(aMsg, aIter, &(aResult->mRtpContributingSourceStats)) ||
> -        !ReadParam(aMsg, aIter, &(aResult->mOfferer))) {
> +        !ReadParam(aMsg, aIter, &(aResult->mOfferer)) ||
> +        !ReadParam(aMsg, aIter, &(aResult->mTrickledIceCandidateStats))
> +       ) {

Preserve existing arbitrary brace at EOL structure.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:694
(Diff revision 1)
> +    captionSpan2.className = "trickled";
> +    caption.appendChild(captionSpan1);
> +    caption.appendChild(captionSpan2);
> +    caption.className = "no-print";
> +
>      let tbody = [];

If you like you can do this with Array.map:
let tbody = [...this.generateICEStats()].map(stat => [
    stat["local-candidate"] || "",
    // ... etc
    stat.bytesReceived || "",
]);

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:697
(Diff revision 1)
> +    caption.className = "no-print";
> +
>      let tbody = [];
> -    for (let stat of this.generateICEStats()) {
> +    let stats = this.generateICEStats();
> +    for (let stat of stats) {
>        tbody.push([

I know this is not part of the patch but this hides values that are 0, which I don't think is intended.
Instead of the \[foo || "", /\* etc.\*/\] pattern, one can:
\[foo, /\* etc. \*/\].map( entry => Object.is(entry, undefined) ? "" : entry)

I would file a bug, or fix it here, or ping me and I will file a bug.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:709
(Diff revision 1)
>          stat.bytesSent || "",
>          stat.bytesReceived || ""
>        ]);
>      }
>  
>      let statsTable = new SimpleTable(

Similarly:
let statsTable = new SimpleTable(["local_candidate", "ice_state", /* etc. */].map(rowName => getString(rowName),
tbody, caption).render();

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:719
(Diff revision 1)
> -
> -    let div = document.createElement("div");
> -    let heading = document.createElement("h4");
> -
> -    heading.textContent = getString("ice_stats_heading");
> -    div.appendChild(heading);
> +      ],
> +      tbody, caption).render();
> +
> +    // after rendering the table, we need to change the class name for each
> +    // candidate pair's local or remote candidate if it was trickled.
> +    [...statsTable.rows].forEach((row, index)=> {

space between arg paren and =>

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:720
(Diff revision 1)
> -    let div = document.createElement("div");
> -    let heading = document.createElement("h4");
> -
> -    heading.textContent = getString("ice_stats_heading");
> -    div.appendChild(heading);
> -
> +      tbody, caption).render();
> +
> +    // after rendering the table, we need to change the class name for each
> +    // candidate pair's local or remote candidate if it was trickled.
> +    [...statsTable.rows].forEach((row, index)=> {
> +      if (index > 0 && stats[index-1]["remote-trickled"]) {

If you want you can spread over the stats and avoid the index check.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:755
(Diff revision 1)
>  
>      for (let candidate of this._report.iceCandidateStats) {
>        candidates.set(candidate.id, candidate);
>      }
>  
> +    let trickled = {};

If you want you can do: 
let isTrickled = id => [...this._report.trickledIceCandidateStats].some(
  candidate => candidate.id == id);

Then you can isTrickled(anId).

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8935953 [details]
Bug 1414169 - pt 5 - Sort candidate pairs that have sent bytes at the top of the table.

https://reviewboard.mozilla.org/r/206802/#review212678

LGTM

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8935954 [details]
Bug 1414169 - pt 6 - Add all raw candidates table (local and remote).

https://reviewboard.mozilla.org/r/206804/#review212680

LGTM with the 1 change that clangbot needs

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8935955 [details]
Bug 1414169 - pt 7 - Refactor folding sections into reusable code.

https://reviewboard.mozilla.org/r/206806/#review212682

LGTM

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:392
(Diff revision 1)
>  
>      if (!this._log || !this._log.length) {
>        return content;
>      }
>  
> -    let div = document.createElement("div");
> +    let div = new FoldableSection(content, {

Nice

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8935956 [details]
Bug 1414169 - pt 8 - Refactor creating html elements for conciseness.

https://reviewboard.mozilla.org/r/206808/#review212686

LGTM with nits.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:443
(Diff revision 1)
>    }
>  };
>  
> +function renderElement(elemName, elemText, options = {}) {
> +  let elem = document.createElement(elemName);
> +  if (elemText != null) {

This check is not necessary.
See https://jsfiddle.net/Lm0evkz6/ for an example

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:446
(Diff revision 1)
> +function renderElement(elemName, elemText, options = {}) {
> +  let elem = document.createElement(elemName);
> +  if (elemText != null) {
> +    elem.textContent = elemText;
> +  }
> +  if (Object.getOwnPropertyNames(options).length) {

I am not sure this check is necessary.

	Nils Ohlmeier [:drno] Reporter
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8935949 [details]
Bug 1414169 - pt 1 - add trickle field to nr_ice_candidate.

https://reviewboard.mozilla.org/r/206794/#review213812

LGTM
Sorry for the too long review time.

	Pulsebot
	Comment 34 • 7 years ago

Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/febf70828830
pt 1 - add trickle field to nr_ice_candidate. r=drno
https://hg.mozilla.org/integration/autoland/rev/72efe90a96b3
pt 2 - aboutwebrtc.js SimpleTable improvements. r=ng
https://hg.mozilla.org/integration/autoland/rev/9445a1ba1361
pt 3 - RTCStatsReport.webidl to support about:webrtc changes for showing trickled candidates. r=ng,smaug
https://hg.mozilla.org/integration/autoland/rev/b907280a1894
pt 4 - Trickled ICE candidates are highlighted with a light blue background. r=ng
https://hg.mozilla.org/integration/autoland/rev/56c460b1034f
pt 5 - Sort candidate pairs that have sent bytes at the top of the table. r=ng
https://hg.mozilla.org/integration/autoland/rev/dbb3a18f3003
pt 6 - Add all raw candidates table (local and remote). r=ng
https://hg.mozilla.org/integration/autoland/rev/81947b1bd8f3
pt 7 - Refactor folding sections into reusable code. r=ng
https://hg.mozilla.org/integration/autoland/rev/af9b67f94aa9
pt 8 - Refactor creating html elements for conciseness. r=ng

	Dorel Luca [:dluca]
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/febf70828830
https://hg.mozilla.org/mozilla-central/rev/72efe90a96b3
https://hg.mozilla.org/mozilla-central/rev/9445a1ba1361
https://hg.mozilla.org/mozilla-central/rev/b907280a1894
https://hg.mozilla.org/mozilla-central/rev/56c460b1034f
https://hg.mozilla.org/mozilla-central/rev/dbb3a18f3003
https://hg.mozilla.org/mozilla-central/rev/81947b1bd8f3
https://hg.mozilla.org/mozilla-central/rev/af9b67f94aa9

	Francesco Lodolo [:flod]
	Comment 36 • 7 years ago
mozreview-review

Comment on attachment 8935952 [details]
Bug 1414169 - pt 4 - Trickled ICE candidates are highlighted with a light blue background.

https://reviewboard.mozilla.org/r/206800/#review214184

::: toolkit/locales/en-US/chrome/global/aboutWebrtc.properties:112
(Diff revision 3)
> +# ICE candidates arriving after the remote answer arrives are considered
> +# trickled (an attribute of an ICE candidate).  These are highlighted in
> +# the ICE stats table with light blue background.  This message is split
> +# into two in order to highlight the word "blue" with a light blue
> +# background to visually match the trickled ICE candidates.
> +trickle_caption_msg = trickled candidates (arriving after answer) are highlighted in

Next time you decide to do something as complex as this, ask for feedback from l10n. This is all but localizable, because it assumes that "blue" is going to be at the end of the sentence.

	Michael Froman [:mjf] Assignee
	Comment 37 • 7 years ago
mozreview-review-reply

Comment on attachment 8935952 [details]
Bug 1414169 - pt 4 - Trickled ICE candidates are highlighted with a light blue background.

https://reviewboard.mozilla.org/r/206800/#review214184

> Next time you decide to do something as complex as this, ask for feedback from l10n. This is all but localizable, because it assumes that "blue" is going to be at the end of the sentence.

I'm happy to take suggestions on how you would handle this case.  If you have a change I can make that would ease localization, I'll open a bug and make the fix.