Closed
Bug 1254839
Opened 8 years ago
Closed 8 years ago
PeerConnection warnings don't include file and line numbers.
Categories
(Core :: WebRTC, defect, P2)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jib, Assigned: jib)
Details
Attachments
(2 files)
It would be helpful if they did, and it is doable.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38869/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38869/
Attachment #8728230 -
Flags: review?(bzbarsky)
Updated•8 years ago
|
Rank: 25
Priority: -- → P2
Comment 2•8 years ago
|
||
Sorry for the lag here. I should be able to get to this on Monday...
Updated•8 years ago
|
Attachment #8728230 -
Flags: review?(bzbarsky) → review+
Comment 3•8 years ago
|
||
Comment on attachment 8728230 [details] MozReview Request: Bug 1254839 - include file and line number in RTCPeerConnection warnings. https://reviewboard.mozilla.org/r/38869/#review36705 ::: dom/media/PeerConnection.js:650 (Diff revision 1) > + > + logWarning: function(msg) { > + this.logStackMsg(msg, Ci.nsIScriptError.warningFlag); > + }, > + > + getNonPeerConnectionFileAndLine: function(stack) { This is really fragile, no? Especially if the stack format ever changes... How about this: var err = new this._win.Error(); var file = err.fileName; var line = err.lineNumber; should work, I believe. The key part is that the doing the Error constructor over the Xray to `_win` will capture only the part of the stack that code in `_win` should see. Alternately, we could add an explicit way to get the info you want (call some xpconnect function, pass in the global whose view of the stack you want, it will snapshot the stack, filter on that principal, return it to you, that sort of thing). But I think the Error xray thing should work. Especially if we add tests. r=me with that issue fixed and a test added to check what we're actually logging in some simple case. Sorry for the lag; I got scared off by the stack-munging and spent a while thinking about how to do it a bit better before realizing that we don't need to do it at all.
Assignee | ||
Comment 4•8 years ago
|
||
Thanks! How do I test whether something gets logged web console? Just making sure I understand.
Flags: needinfo?(bzbarsky)
Comment 5•8 years ago
|
||
> How do I test whether something gets logged web console? The best thing is probably a browser test in which you implement a console listener, then load some page that should trigger a warning in a tab. See http://mxr.mozilla.org/mozilla-central/source/layout/style/test/chrome/test_bug1160724.xul for an example of implementing a console listener.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8728230 [details] MozReview Request: Bug 1254839 - include file and line number in RTCPeerConnection warnings. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38869/diff/1-2/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40285/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40285/
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8730971 [details] MozReview Request: Bug 1254839 - add test for RTCPeerConnection deprecation warnings. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40285/diff/1-2/
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8728230 [details] MozReview Request: Bug 1254839 - include file and line number in RTCPeerConnection warnings. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38869/diff/2-3/
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8730971 [details] MozReview Request: Bug 1254839 - add test for RTCPeerConnection deprecation warnings. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40285/diff/2-3/
Assignee | ||
Comment 11•8 years ago
|
||
Rebased since try was such a mess, and media mochitests tests have moved.
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8730971 [details] MozReview Request: Bug 1254839 - add test for RTCPeerConnection deprecation warnings. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40285/diff/3-4/
Assignee | ||
Comment 13•8 years ago
|
||
unregisterListener would help.
Assignee | ||
Updated•8 years ago
|
Attachment #8730971 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•8 years ago
|
||
Green try. Feel free to forward review if you have someone in mind, or bounce. I wasn't sure who to get.
Comment 15•8 years ago
|
||
Comment on attachment 8730971 [details] MozReview Request: Bug 1254839 - add test for RTCPeerConnection deprecation warnings. https://reviewboard.mozilla.org/r/40285/#review37351 r=me assuming this works right in e10s.
Attachment #8730971 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 16•8 years ago
|
||
e10s is green also.
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b74f55997272 https://hg.mozilla.org/integration/mozilla-inbound/rev/a36185985ee8
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b74f55997272 https://hg.mozilla.org/mozilla-central/rev/a36185985ee8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•