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)

defect

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.
Rank: 25
Priority: -- → P2
Sorry for the lag here.  I should be able to get to this on Monday...
Attachment #8728230 - Flags: review?(bzbarsky) → review+
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.
Thanks! How do I test whether something gets logged web console? Just making sure I understand.
Flags: needinfo?(bzbarsky)
> 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)
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/
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/
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/
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/
Rebased since try was such a mess, and media mochitests tests have moved.
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/
unregisterListener would help.
Attachment #8730971 - Flags: review?(bzbarsky)
Green try. Feel free to forward review if you have someone in mind, or bounce. I wasn't sure who to get.
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+
e10s is green also.
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.