Closed Bug 1138336 Opened 5 years ago Closed 5 years ago

Adapt nsIScriptError so it can display log messages with filename and line number

Categories

(Toolkit Graveyard :: Error Console, enhancement)

enhancement
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch logmessage-1.diff (obsolete) — Splinter Review
(I really don't know if this is the right component but it'll do.)

It should be easy to post log messages with file and line number information attached. nsIScriptError is perfect for this but it only handles errors and warnings.

With this patch I add a new flag to nsIScriptError for info messages, and adapt the old and new consoles to display them correctly.

I'm going to need review from XPConnect, DevTools and Toolkit teams, but first up I'll ask for review on the XPConnect parts since that's most critical.
Attachment #8571236 - Flags: review?(jst)
Comment on attachment 8571236 [details] [diff] [review]
logmessage-1.diff

r=jst for the xpconnect bits.
Attachment #8571236 - Flags: review?(jst) → review+
Comment on attachment 8571236 [details] [diff] [review]
logmessage-1.diff

Neil, please review the toolkit/components/console changes.

While I was there I noticed that this part only works because warningFlag is 1. Good job it's never been changed. :)

> var warning = aObject.flags & nsIScriptError.warningFlag != 0;
Attachment #8571236 - Flags: review?(neil)
(In reply to Geoff Lankow from comment #2)
> While I was there I noticed that this part only works because warningFlag is
> 1. Good job it's never been changed. :)
> 
> > var warning = aObject.flags & nsIScriptError.warningFlag != 0;

Wow. I did some CVS archaeology. The line
var warning = scriptError.flags & nsIScriptError.WARNING != 0;
was originally checked in on 2000-03-21 as version 1.4 of console.js and has since been searched and replaced and copied and pasted sevceral times before getting to where you see it.
Comment on attachment 8571236 [details] [diff] [review]
logmessage-1.diff

> .console-row[type="error"],
>-.console-row[type="warning"] {
>+.console-row[type="warning"],
>+.console-row[type="message"] {
>   -moz-binding: url("chrome://global/content/consoleBindings.xml#error");
> }
> 
>-.console-row[type="message"] {
>+.console-row[type="message"]:not([href]) {
>   -moz-binding: url("chrome://global/content/consoleBindings.xml#message");
> }
Would it work to use .console-row[type="message"][href] in the first block (which would make it more specific, or you could exchange the rules if you prefer)? (Any particular reason you chose href anyway?)

>-          var warning = aObject.flags & nsIScriptError.warningFlag != 0;
>+          var message = !!(aObject.flags & nsIScriptError.infoFlag);
>+          var warning = !!(aObject.flags & nsIScriptError.warningFlag);
> 
>-          var typetext = warning ? "typeWarning" : "typeError";
>+          var typetext = message ? "typeMessage" : (warning ? "typeWarning" : "typeError");
>           row.setAttribute("typetext", this.mStrBundle.getString(typetext));
>-          row.setAttribute("type", warning ? "warning" : "error");
>+          row.setAttribute("type", message ? "message" : (warning ? "warning" : "error"));
Would it make sense to start using the logLevel instead of these nested ternary operators?
Ugh, looks like I never tidied up after doing the minimum required to make it work. This looks prettier.
Attachment #8571236 - Attachment is obsolete: true
Attachment #8571236 - Flags: review?(neil)
Attachment #8573017 - Flags: review?(neil)
Attachment #8573017 - Flags: review?(neil) → review+
Comment on attachment 8573017 [details] [diff] [review]
logmessage-2.diff

Mihai, review on the devtools bits please.
Attachment #8573017 - Flags: review?(mihai.sucan)
Attachment #8573017 - Flags: review?(mihai.sucan) → review?(past)
Comment on attachment 8573017 [details] [diff] [review]
logmessage-2.diff

Review of attachment 8573017 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8573017 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/52c23bbb3009
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.