Closed
Bug 1414169
Opened 7 years ago
Closed 7 years ago
Show received ICE candidates on about:webrtc
Categories
(Core :: WebRTC, enhancement, P2)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: drno, Assigned: mjf)
References
(Blocks 1 open bug)
Details
Attachments
(8 files)
59 bytes,
text/x-review-board-request
|
drno
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
ng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ng
:
review+
|
Details |
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mfroman
Rank: 19
Priority: P3 → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Attachment #8935950 -
Flags: review?(na-g) → review+
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+
Attachment #8935951 -
Flags: review?(bugs) → review+
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 &
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
Attachment #8935951 -
Flags: review?(na-g) → review+
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).
Attachment #8935952 -
Flags: review?(na-g) → review+
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
Attachment #8935953 -
Flags: review?(na-g) → review+
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
Attachment #8935954 -
Flags: review?(na-g) → review+
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
Attachment #8935955 -
Flags: review?(na-g) → review+
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.
Attachment #8935956 -
Flags: review?(na-g) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Attachment #8935949 -
Flags: review?(drno) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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.
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.
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•