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

Status: RESOLVED FIXED

(Toolkit Graveyard :: Error Console, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Attachment: logmessage-1.diff

(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.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 8571236 [details] [diff] [review]
logmessage-1.diff

r=jst for the xpconnect bits.

Comment 2

[Geoff Lankow (:darktrojan)]

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;

Comment 3

[[e-mail]]

(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 4

[[e-mail]]

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?

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: logmessage-2.diff

Ugh, looks like I never tidied up after doing the minimum required to make it work. This looks prettier.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 8573017 [details] [diff] [review]
logmessage-2.diff

Mihai, review on the devtools bits please.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8573017 [details] [diff] [review]
logmessage-2.diff

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

Looks good to me.

Comment 8

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52c23bbb3009

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52c23bbb3009