489736 - Make the error console more like the rest of toolkit apps

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: Error Console, enhancement)

Description

[Nochum Sossonko [:Natch]]

Attachment: wip

Gonna try to refresh the error console a bit.

This patch is still missing quite a bit, namely I need to hook up the eval textbox and throw in some styles for gnomestripe and pinstripe.

Comment 1

[Nochum Sossonko [:Natch]]

Dao, do you know who I can ask to review the error console code?

Also, why isn't the error console in mozapps? I can file another bug to move it there, once this is done. Should it be there?

The patch is alot of moving stuff around (eg xml->js) and gives the error console the mozapp look. I also stuck in a pair of checkboxes for the two prefs (strict and showInConsole). I didn't hook up the strict pref yet to really do anything, I'll address that in the next patch. The editing commands are also up and working in this patch. I was thinking of adding cmd_delete, but that wouldn't really be of any use since there is no real way of deleting a single message from the service AFAICT.

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Dao, do you know who I can ask to review the error console code?

According to blame, Simon Bünzli would be the best candidate, and then Gavin or Neil R. for the official blessing.

Comment 3

[Philip Chee]

This is not a review (I'm only pretending to be a reviewer here).

-  <toolbox id="console-toolbox" chromedir="&locale.dir;">

SeaMonkey overlays the toolkit Error Console toolbox to insert our application wide menubar. Removing the toolbox makes things slightly more inconvenient for us. CC some more SeaMonkey people.

+      <radiogroup id="viewGroup" persist="selectedItem" orient="horizontal"
+                  class="viewSelector chromeclass-toolbar"
+                  chromedir="&locale.dir;">

Um we carefully removed |chromedir="&locale.dir;"| from the Error Console earlier. Why are you reintroducing this bug?

+        <radio id="console-all" label="&all.label;" persist="selected"
+               onclick="gConsole.showTab('all')"
+               accesskey="&all.accesskey;"/>

I perfer one attribute per line and to keep the label and accesskey together.

-  <iframe name="Evaluator" id="Evaluator" collapsed="true"/>
Err what happened here??

+  <hbox pack="end">
+    <checkbox id="showChrome" label="&showChrome.label;"
+              accesskey="&showChrome.accesskey;"
+              oncommand="gConsole.togglePref(gConsole.chromePref, this.checked)"/>
+    <checkbox id="showStrict" label="&showStrict.label;"
+              accesskey="&showStrict.accesskey;"
+              oncommand="gConsole.togglePref(gConsole.strictPref, this.checked)"/>
+    <button id="clearButton" label="&clear.label;"
+            accesskey="&clear.accesskey;"
+            onclick="gConsole.clear()"/>
+  </hbox>

Why is this UI at the bottom? and why checkboxes?

+var gConsole = {
+  interfaces: [Components.interfaces.nsIContentPrefObserver,
+               Components.interfaces.nsISupportsWeakReference,
+               Components.interfaces.nsISupports],
....
+  QueryInterface: function FullZoom_QueryInterface(aIID) {
+    if (!this.interfaces.some(function (v) aIID.equals(v)))
+      throw Cr.NS_ERROR_NO_INTERFACE;
+    return this;
+  },
You only reference this.interfaces once. Hardly worth the effort.

+  get csrv() {
+    delete this.csrv;
+    return this.csrv =
+      Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService);
Long gone are the days of the unix command line and cryptic commands like awk, sed, grep, cat. Try something more informative like consoleService.

+  get prefServ() {
prefs or prefBranch.

+  get clipboard() {
You only reference this once. Why have a separate getter?

+  chromePref: "javascript.options.showInConsole",
+  strictPref: "javascript.options.strict",

If you are going to use nsIPrefBranch2 you can reduce these to:
chromePref: "showInConsole",
strictPref: "strict",

+  togglePref: function console__toggleShowChrome(aPref, aShouldShow) {
+    switch(aPref) {
+      case this.chromePref:
+        if (aShouldShow != this.showChrome)
+          this.prefServ.setBoolPref(aPref, !!aShouldShow);
+        break;
+      case this.strictPref:
+        if (aShouldShow != this.showStrict)
+          this.prefServ.setBoolPref(aPref, !!aShouldShow);
+        break;
+    }
+  },
+
+  observe: function console__observe(aSubject, aTopic, aData) {
+    switch(aTopic) {
+      case "nsPref:changed":
+        switch(aData) {
+          case this.chromePref:
+            this.showChrome =
+              this._prefBranch.getBoolPref(this.chromePref);
+            break;
+          case this.strictPref:
+            this.showStrict =
+              this._prefBranch.getBoolPref(this.strictPref);
+            break;
+        }
+    }
+  },
You need to toggle the checkboxes in the observer as well as the prefs can be changed from elsewhere e.g. about:config.

+    try {
+      var scriptError = aMessage.QueryInterface(Ci.nsIScriptError);
....
+      try {
+        var msg = aMessage.QueryInterface(Ci.nsIConsoleMessage);

Don't use try. Do something that is guaranteed not to throw like:

if (aMessage instanceof Ci.nsIScriptError)
  {...}
else if (aMessage instanceof Ci.nsIConsoleMessage)
  {...}
else
  {...}

+    var char = "";
+    var tenSpace = "          ";
+    var fiveSpace = "     ";
Eeek. Try to use CSS + XUL for formatting.

+        "\r\n\r\n" + aItem.errorSrcStart.value + aItem.errorSrcEnd.value + tenSpace;
What about *nix and OSX and BeOS and Solaris?

+    var separator = "\r\n________________________________________\r\n\r\n";
Double Eeek.

-<!DOCTYPE bindings SYSTEM "chrome://global/locale/console.dtd">
-
+<!DOCTYPE window [
+  <!ENTITY % console SYSTEM "chrome://global/locale/console.dtd"> %console;
+]>
Why this change?

+  <binding id="console-message-error"
+    extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
Line up extends with the id.

+#viewGroup radio:hover {
+  background-color: #E0E8F6;
+  color: black;
Don't use hard coded colours. Try to find a suitable system colour.

+.console-source {
+  color: #0000BB;
Ditto, etc.

Comment 4

[Robert Kaiser]

Could you show a screenshot of what this looks like? I smell the SeaMonkey overlay getting much heavier with this rework...

Comment 5

[Nochum Sossonko [:Natch]]

(In reply to comment #3)
> This is not a review (I'm only pretending to be a reviewer here).

Comments are always welcome, thanks! Note, though, that this is really only a wip patch, besides the aforementioned bugs with it there are others I've identified that I will fix in the next patch.

> -  <toolbox id="console-toolbox" chromedir="&locale.dir;">
> 
> SeaMonkey overlays the toolkit Error Console toolbox to insert our application
> wide menubar. Removing the toolbox makes things slightly more inconvenient for
> us. CC some more SeaMonkey people.

Not a problem, I can put it back.
> +      <radiogroup id="viewGroup" persist="selectedItem" orient="horizontal"
> +                  class="viewSelector chromeclass-toolbar"
> +                  chromedir="&locale.dir;">
> 
> Um we carefully removed |chromedir="&locale.dir;"| from the Error Console
> earlier. Why are you reintroducing this bug?

Why should there not be a chromedir? Every other app uses this chromedir, why shouldn't the error console? Can you point me to the bug?
[...snip...]
> -  <iframe name="Evaluator" id="Evaluator" collapsed="true"/>
> Err what happened here??

Not hooked up yet, I redid the xul/xml/js/css more or less from scratch, this will come back!
> +  <hbox pack="end">
> +    <checkbox id="showChrome" label="&showChrome.label;"
> +              accesskey="&showChrome.accesskey;"
> +              oncommand="gConsole.togglePref(gConsole.chromePref,
> this.checked)"/>
> +    <checkbox id="showStrict" label="&showStrict.label;"
> +              accesskey="&showStrict.accesskey;"
> +              oncommand="gConsole.togglePref(gConsole.strictPref,
> this.checked)"/>
> +    <button id="clearButton" label="&clear.label;"
> +            accesskey="&clear.accesskey;"
> +            onclick="gConsole.clear()"/>
> +  </hbox>
> 
> Why is this UI at the bottom? and why checkboxes?

Similar to the download manager, I put it at the bottom. IMHO it fits alot better there. What should I use (if not checkboxes)?
[...snip...]
> +  get clipboard() {
> You only reference this once. Why have a separate getter?

Usually, getService calls are placed in a getter so as not to hurt startup time. This is reused on every copy.
> +  chromePref: "javascript.options.showInConsole",
> +  strictPref: "javascript.options.strict",
> 
> If you are going to use nsIPrefBranch2 you can reduce these to:
> chromePref: "showInConsole",
> strictPref: "strict",

will do.
> +  togglePref: function console__toggleShowChrome(aPref, aShouldShow) {
> +    switch(aPref) {
> +      case this.chromePref:
> +        if (aShouldShow != this.showChrome)
> +          this.prefServ.setBoolPref(aPref, !!aShouldShow);
> +        break;
> +      case this.strictPref:
> +        if (aShouldShow != this.showStrict)
> +          this.prefServ.setBoolPref(aPref, !!aShouldShow);
> +        break;
> +    }
> +  },
> +
> +  observe: function console__observe(aSubject, aTopic, aData) {
> +    switch(aTopic) {
> +      case "nsPref:changed":
> +        switch(aData) {
> +          case this.chromePref:
> +            this.showChrome =
> +              this._prefBranch.getBoolPref(this.chromePref);
> +            break;
> +          case this.strictPref:
> +            this.showStrict =
> +              this._prefBranch.getBoolPref(this.strictPref);
> +            break;
> +        }
> +    }
> +  },
> You need to toggle the checkboxes in the observer as well as the prefs can be
> changed from elsewhere e.g. about:config.

This is a bit borked, I meant to handle everything in togglePref, forgot to properly set it up, will fix.
> +    try {
> +      var scriptError = aMessage.QueryInterface(Ci.nsIScriptError);
> ....
> +      try {
> +        var msg = aMessage.QueryInterface(Ci.nsIConsoleMessage);
> 
> Don't use try. Do something that is guaranteed not to throw like:
> 
> if (aMessage instanceof Ci.nsIScriptError)
>   {...}
> else if (aMessage instanceof Ci.nsIConsoleMessage)
>   {...}
> else
>   {...}
Will do, this was mostly copy/paste.
> +    var char = "";
> +    var tenSpace = "          ";
> +    var fiveSpace = "     ";
> Eeek. Try to use CSS + XUL for formatting.

I'm not sure I understand this, how can I do that for the clipboard? I need ascii text, that can be pasted in a regular textbox.
> +        "\r\n\r\n" + aItem.errorSrcStart.value + aItem.errorSrcEnd.value +
> tenSpace;
> What about *nix and OSX and BeOS and Solaris?

Not sure what to do about that actually (you're talking about the double "\r\n", right?)
[...snip...p
> -<!DOCTYPE bindings SYSTEM "chrome://global/locale/console.dtd">
> -
> +<!DOCTYPE window [
> +  <!ENTITY % console SYSTEM "chrome://global/locale/console.dtd"> %console;
> +]>
> Why this change?

No reason, will fix :)
[...snip...]
> +#viewGroup radio:hover {
> +  background-color: #E0E8F6;
> +  color: black;
> Don't use hard coded colours. Try to find a suitable system colour.
This is how all the other apps do it, copy/paste.
> +.console-source {
> +  color: #0000BB;
> Ditto, etc.

Will try, although I don't think there's an appropriate color for this (-moz-nativehyperlink, but that's too dark, I think).

@Robert: I'll try to attach one today to this bug.

Comment 6

[Nochum Sossonko [:Natch]]

Attachment: screenshot

Comment 7

[Philip Chee]

>> Um we carefully removed |chromedir="&locale.dir;"| from the Error Console
>> earlier. Why are you reintroducing this bug?

> Why should there not be a chromedir? Every other app uses this chromedir, why
> shouldn't the error console? Can you point me to the bug?
Oh sorry, I was thinking of the textbox in Bug 479588 which is a uri-element. But I believe that Mozilla has plans in the future to remove chromedir= so I'm not sure if adding more would be useful at this stage.

Comment 8

[Nochum Sossonko [:Natch]]

> Oh sorry, I was thinking of the textbox in Bug 479588 which is a uri-element.
> But I believe that Mozilla has plans in the future to remove chromedir= so I'm
> not sure if adding more would be useful at this stage.

That's bug 478416, until that's fixed this is really the only option. I'll keep an eye out, and will remove it once it's fixed.

Comment 9

[Mike Connor [:mconnor]]

(In reply to comment #1)
> Also, why isn't the error console in mozapps? I can file another bug to move it
> there, once this is done. Should it be there?

Eh, mozapps is weird, I'd rather move everything into components for sanity (most or all stuff in mozapps also has code in components).

> The patch is alot of moving stuff around (eg xml->js) and gives the error
> console the mozapp look.

What's the "mozapp look"?  This looks kinda like the Firefox 2 dlmgr, I guess.

> I was thinking of adding cmd_delete, but that
> wouldn't really be of any use since there is no real way of deleting a single
> message from the service AFAICT.

You could fix that... :)

Comment 10

[Nochum Sossonko [:Natch]]

(In reply to comment #9)
> What's the "mozapp look"?  This looks kinda like the Firefox 2 dlmgr, I guess.

Well it makes it look pretty much like the rest of chrome windows/dialogs. The Page Info, Addons Manager, Options Dialog, etc. all have the radiogroup look that I copied over here. The richlistbox is just a better way of expressing the error console. The button and checkboxes on the bottom is kind of like the download manager.

> > I was thinking of adding cmd_delete, but that
> > wouldn't really be of any use since there is no real way of deleting a single
> > message from the service AFAICT.
> 
> You could fix that... :)

Will try, although probably in a separate bug, unless it's simple enough...

Comment 11

[Nochum Sossonko [:Natch]]

Attachment: wip 2

Getting Closer!

This patch fixes the evaluation code and defines gBrowser, gNavToolbox, gURLBar and gFindBar for easy accessibility. It also contains various fixes for the code and addresses most of the comments.

Comment 12

[Bryan]

Will this take into account if I have customized my toolbars to use small icons?  If I do have small icons enabled, will the Error Console also show small icons?  It would be great to finally get consistency here.

Thanks for your work though!

~B

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #11)
> defines gBrowser, gNavToolbox, gURLBar and gFindBar for easy accessibility.

You don't want to do that in toolkit...

I don't understand why you have an iframe if it only provides a method that calls eval(). I suppose that's temporary?

Comment 14

[Nochum Sossonko [:Natch]]

Problem is there is no other place to run chrome code, the error console is the best place for it. It is fault tolerant (i.e. if there is no window opened at the time) in that it just won't be defined. I suppose I should explicitly null them out if there is no browser window, will do that in the next patch.

The separate iframe is so that if someone punches in gConsole = null or some such it shouldn't ruin the error console. I don't see any other way of running code successfully otherwise (and the javascript: url hack, which also used an iframe, is far worse IMHO).

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> Problem is there is no other place to run chrome code, the error console is the
> best place for it. It is fault tolerant (i.e. if there is no window opened at
> the time) in that it just won't be defined. I suppose I should explicitly null
> them out if there is no browser window, will do that in the next patch.

Please just leave out browser-specific code. What I think you could do is define a generic variable that points to window.opener (where window is the console window).

> The separate iframe is so that if someone punches in gConsole = null or some
> such it shouldn't ruin the error console.

I don't see how the iframe element would prevent that. You're still calling eval in the same window, as far as I can see.

Comment 16

[Philip Chee]

I think that this patch tries to do too many things and should be split into several smaller incremental patches; at least:

1. UI Refresh. Addressing the toolkit "look and feel", radio buttons, styling etc.
2. Support multiselect, cut and paste and moving to using a richlistbox.
3. Showing more information in each row in the Error Console.
4. Changing the way js is evaluated.

(In reply to comment #13)
> (In reply to comment #11)
>> defines gBrowser, gNavToolbox, gURLBar and gFindBar for easy accessibility.

> You don't want to do that in toolkit...

Definitely NOT. There are toolkit based browsers that aren't called "Firefox". There are even more toolkit based apps that aren't even "browsers".

> I don't understand why you have an iframe if it only provides a method that
> calls eval(). I suppose that's temporary?

Any changes to the way the js code in the textbox is evaluated needs to be run past the Error Console experts. There have been regressions in the past when people tried to "simplify" the code.

(In reply to comment #15)
> What I think you could do is
> define a generic variable that points to window.opener (where window is the
> console window).

You can open an error console without opening any other window I think. Something like:
    seamonkey.exe -silent -jsconsole
In which case window.opener is null.

Also I don't see the point of getting the window.opener, why do you need this anyway?

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Any changes to the way the js code in the textbox is evaluated needs to be run
> past the Error Console experts. There have been regressions in the past when
> people tried to "simplify" the code.
*ugh*

I bet nobody has bothered to write tests either...

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #16)
> > What I think you could do is
> > define a generic variable that points to window.opener (where window is the
> > console window).
> 
> You can open an error console without opening any other window I think.
> Something like:
>     seamonkey.exe -silent -jsconsole
> In which case window.opener is null.

Which is fine, because you obviously don't want to access the host application in that case.

> Also I don't see the point of getting the window.opener, why do you need this
> anyway?

I need it because I want to access that window and play with it. Right now I can use top.opener, which is awkward and a hack based on the iframe implementation.

Comment 19

[Philip Chee]

>> Also I don't see the point of getting the window.opener, why do you need this
>> anyway?
> I need it because I want to access that window and play with it. Right now I
> can use top.opener, which is awkward and a hack based on the iframe
> implementation.

Yes but why would the average Error Console user need it?
And if they do why don't they install an extension like Execute JS:
<https://addons.mozilla.org/en-US/firefox/addon/1729>

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> >> Also I don't see the point of getting the window.opener, why do you need this
> >> anyway?
> > I need it because I want to access that window and play with it. Right now I
> > can use top.opener, which is awkward and a hack based on the iframe
> > implementation.
> 
> Yes but why would the average Error Console user need it?

The average user would neither need, know about nor use it.

Comment 21

[Philip Chee]

(In reply to comment #20)
>>>> Also I don't see the point of getting the window.opener, why do you need this
>>>> anyway?
>>> I need it because I want to access that window and play with it. Right now I
>>> can use top.opener, which is awkward and a hack based on the iframe
>>> implementation.
>> Yes but why would the average Error Console user need it?

> The average user would neither need, know about nor use it.

You know if I tried that sort of reasoning in comm-central/suite I'd get slapped down so hard that all that would be left of me would be a wet spot on the ground. Perhaps toolkit is different?

Comment 22

[Mike Connor [:mconnor]]

We're talking about the behaviour of a textbox for evaluating javascript snippets.  If you're arguing for "what would an average user need?" as your standard for inclusion, we could take a pretty big hacksaw to this window, or kill it entirely, since the "average" user has no clue what any of this UI talks about at all.

Comment 23

[Philip Chee]

I'm sorry. You're right. I shouldn't be bike-shedding over trivial issues. How about expanding this so that you can access any open window and not just the opener? I frequently have multiple windows open and you can't open the Error Console from some windows e.g. the Places Library, Download Manager.

Comment 24

[Nochum Sossonko [:Natch]]

I have looked through blame to see why certain methods were used for the code evaluation, it seemed mostly to be about bugs https://bugzilla.mozilla.org/buglist.cgi?bug_id=369097,380422,385092 . All stem from the fact that a javscript: url was being used. I have to ways of fixing this (and the problem in comment 15, which was my mistake. Either through eval(code, object) or by going through the contentWindow, I'll probably use the latter (if it works correctly) being that the former is really supposed to be deprecated...

As for the defined constants, I don't really see why so many people are against it, either it will work (e.g. if there is a browser window) and make life easier, or it won't. I'll remove it from the code though...

TODO:

* Fixup pinstripe and gnomestripe.
* Fixup sorting (with some new options hopefully)

Thanks for all the helpful comments guys!

Comment 25

[Philip Chee]

> As for the defined constants, I don't really see why so many people are against
> it, either it will work (e.g. if there is a browser window) and make life

The Error Console is a toolkit component. I think it is considered bad practice to hard code knowledge of the internals of one specific consumer (albeit a significant one) of toolkit code. You could create some sort of generic callback that consumers can subscribe to (or not) if they wish.

Comment 26

[Dão Gottwald [:dao]]

gBrowser, gNavToolbox, gURLBar and gFindBar seem to be arbitrary choices. Even if there was an API for the consumer to add custom properties, I think just giving easy access to the window would be preferable.

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> TODO:
> * Fixup pinstripe and gnomestripe.
> * Fixup sorting (with some new options hopefully)
* Create tests for new functionality at a minimum, and probably for some of those past regressions so you can prove that you didn't break them either.

Comment 28

[Nochum Sossonko [:Natch]]

Attachment: wip 3

TODO:

* Tests!
* Pinstripe and Gnomestripe :( hard to do without a mac or linux box.

Iframe is fully functional now, no pre-defined values. window.opener seemed pretty useless to me as 1) it's easily accessible via top.opener and 2) it's easily lost when the browser window that opened the console is closed. I also changed the crufty message text copying I had before to return basically the same thing as the old error console used to, minus the "\r\n" instead of "\n", is that a problem on linux/mac?

Put back the toolbox as well for the Seamonkey folks. Let me know if that should suffice.

Comment 29

[[e-mail]]

If memory serves me right, one advantage of using the iframe instead of eval is that console.js doesn't appear on the JavaScript stack (for errors etc.)

Comment 30

[Nochum Sossonko [:Natch]]

(In reply to comment #29)
> If memory serves me right, one advantage of using the iframe instead of eval is
> that console.js doesn't appear on the JavaScript stack (for errors etc.)

...which with this hack still seams to be the case, can you explain in more detail how this differs from the other approach? The "Source File" for the error (when bad code is typed into the eval code box) is: javascript:var%20evaluate%20=%20function(aCode)%20{%20return%20eval(aCode)%20}.

Basically, this is the same exact thing as before (new global window) only without having to wait for loading, and then selecting the result off of the page. There currently is an issue with the eval code anyways, which I'll be fixing and adding a test for, in that if the result of the code is false, or anything that evaluates to false, it won't be displayed. The reason is that when logStringMessage is passed null or the empty string it will clear all the errors, but the check for that should explicitly check for null or "" as opposed to just truthiness.

Please let me know if there are any other issues with the new approach to the code evaluation.

Comment 31

[Dão Gottwald [:dao]]

I suppose this will break further evaluations: location = "http://google.com"

Also note that not sanitizing the iframe for every evaluation is a behavior change in that a new evaluation could be affected by a previous one.

Comment 32

[Nochum Sossonko [:Natch]]

(In reply to comment #31)
> I suppose this will break further evaluations: location = "http://google.com"

Only if evaluate is redeclared :) In the old console doing that would do much the same, there's more of a concern of loading that site with chrome privileges though.
> Also note that not sanitizing the iframe for every evaluation is a behavior
> change in that a new evaluation could be affected by a previous one.

Good point, I can sanitize it if it seems to be a problem (can't see how, the console isn't really made for excessive coding).

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> (In reply to comment #31)
> > I suppose this will break further evaluations: location = "http://google.com"
> 
> Only if evaluate is redeclared :)

It won't be declared at all, as far as I can see.

> > Also note that not sanitizing the iframe for every evaluation is a behavior
> > change in that a new evaluation could be affected by a previous one.
> 
> Good point, I can sanitize it if it seems to be a problem (can't see how, the
> console isn't really made for excessive coding).

I'm not necessarily speaking of problems but a behavior change that we should at least be aware of. E.g. evaluating first 'x = 1' and then 'typeof x' would be different.

Comment 34

[Nochum Sossonko [:Natch]]

I meant only if google has a global function or variable named "evaluate" will the evaluation break, both of the problems can be fixed with sanitizing though, so will do.

Comment 35

[Dão Gottwald [:dao]]

I could as well have written location = "about:blank". There doesn't have to be any JS on the new page. My point is that iframe.contentWindow.evaluate wouldn't exist anymore.
Evaluating 'delete evaluate' would have the same effect.

Comment 36

[Nochum Sossonko [:Natch]]

Ah, well 'delete evaluate' doesn't work in my testing, although the location = about:blank does, but only after it's loaded (i.e. I can get a few evaluations in afterwards), evaluate = null is also problematic, so I'll just sanitize it. It's very simple to do anyways.

Comment 37

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 1

Ready for review.

The functional changes between this patch are:

* Implemented a buffer for the code input line
* Fixed various bugs with the frame and the code evaluator
* Fixed up pinstripe and gnomestrip to the best of my knowledge, can't test it though

Tests are in bug 490499.


Simon, if you're not the right guy to ask for review, please let me know who would be better.

Comment 38

[Simon Bünzli]

Could you please split up this patch into what is the refactoring needed for the richlistbox change (which could also happen e.g. in bug 305206) and what are the functional and styling changes (or even further as suggested by Phil in comment #16). You could use a patch queue and then just indicate which patch has to be applied on top of which other. That would make reviewing simpler and quicker. FYI: I won't get to it before the week-end, anyway.

Comment 39

[Nochum Sossonko [:Natch]]

Comment on attachment 375132 [details] [diff] [review]
patch ver. 1

Gonna work on splitting this patch up into several smaller patches as best as possible. Is it better to split them out to the various other bugs, or is it ok for me to keep it all in this bug?

Comment 40

[Simon Bünzli]

Ideally you'd split it up into several bugs, as some things (as the richlistbox change) will be less debatable than others (as the visible UI changes). With everything in one bug, everything will have to land the same time or status tracking can get quite hairy...

Comment 41

[u88484]

Nochum, I can't really tell what all your trying to fix in this bug but here are a few bug that would be nice to have fixed for the error console...some more than others.  I know a lot of these are way out of the scope of this bug but may also be easy to fix while your at it in this bug.

bug 53480, bug 67603, bug 68951, bug 80702, bug 80704, bug 81209, bug 83019, bug 88057, bug 122213, bug 136672, bug 136681, bug 261621, bug 305206, bug 307354, bug 311309 and bug 312962

Comment 42

[Nochum Sossonko [:Natch]]

Thanks Kurt, I'll have a look at those bugs after this initial landing, some may be fixed others will not be.

Comment 43

[Nochum Sossonko [:Natch]]

Comment on attachment 374274 [details]
screenshot

Layout and appearance should still be the same, this is a screenshot of how it looks on Vista.

Comment 44

[Nochum Sossonko [:Natch]]

Attachment: part 2: give it the new look

This is the part that concerns this bug. I also put in the new checkboxes with this bug, small extra advantage since the clear button was to be moved anyways.

Comment 45

[Philip Chee]

> +<!ENTITY showChrome.label      "Show Internal Errors">
"Internal Errors" sounds rather ambiguous. Is there a particular reason we are avoiding the "Chrome" word?

Comment 46

[Simon Bünzli]

Comment on attachment 375296 [details] [diff] [review]
part 2: give it the new look

I'm not sure about these changes. Please get UI-review first here.

>+#viewGroup radio {
>+  -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#viewbutton");

You're using tabs here in a way we don't anywhere else. I'll leave it to our UI gurus to decide, but IMO we shouldn't do this.

>+  strictPref: "strict",

Either inline it, or alternatively use a constant (same for chromePref). BTW: It should be strictPrefName.

>+  bufferIndex: 0,
>+  buffer: [],

Unused? Wrong patch?

>+  togglePref: function console__toggleShowChrome(aPref, aShouldShow) {

This doesn't toggle but just set. Then again, you could simplify the whole function to

+ this.prefBranch.setBoolPref(aPref, !!aShouldShow);

>+    switch(aTopic) {
>+      case "nsPref:changed":

Nit: Space after switch; and you don't have to indent the case lines.

>+  },
> }

Wrong patch? Else nit: trailing comma.

>+    <key id="key_focus"

Focus what? key_focus_codefield ?

>+  </windowdragbox>
>+  <toolbar class="chromeclass-toolbar" id="ToolbarEval" align="center" nowindowdrag="true">

Shouldn't either the toolbar be inside the windowdragbox - or there's no need for nowindowdrag="true"?

>+    <checkbox id="showChrome"
>+              label="&showChrome.label;"
>+              accesskey="&showChrome.accesskey;"
>+              oncommand="gConsole.togglePref(gConsole.chromePref, this.checked)"/>
>+    <checkbox id="showStrict"
>+              label="&showStrict.label;"
>+              accesskey="&showStrict.accesskey;"
>+              oncommand="gConsole.togglePref(gConsole.strictPref, this.checked)"/>

Do we really want to expose these? IMO we should rather just default showInConsole to true (resp. drop it completely). And the strict bit will only unnecessarily cause bug reports by users accidentally enabling it...

>+<!ENTITY showChrome.label      "Show Internal Errors">

This sounds like a toggle but isn't (except if you close and reopen the console).

>+<!ENTITY showStrict.label      "Show Strict Warnings">

This will never work as a toggle. s/Show/Record/ or something.

Comment 47

[Nochum Sossonko [:Natch]]

(In reply to comment #46)
> I'm not sure about these changes. Please get UI-review first here.

will do, I have the screenshot tagged for ui-review.
> >+#viewGroup radio {
> >+  -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#viewbutton");
> 
> You're using tabs here in a way we don't anywhere else. I'll leave it to our UI
> gurus to decide, but IMO we shouldn't do this.

I don't understand, which way am I adding tabs that isn't done already? Every window that has multiple viewports in Firefox has this exact setup. To name a couple: options dialog and page info dialog. What's more, the addons-window is nearly the exact same thing, only they rebuild the view with a template, something which I would like to do sometime soon, as opposed to hiding/showing with css.

> >+    <checkbox id="showChrome"
> >+              label="&showChrome.label;"
> >+              accesskey="&showChrome.accesskey;"
> >+              oncommand="gConsole.togglePref(gConsole.chromePref, this.checked)"/>
> >+    <checkbox id="showStrict"
> >+              label="&showStrict.label;"
> >+              accesskey="&showStrict.accesskey;"
> >+              oncommand="gConsole.togglePref(gConsole.strictPref, this.checked)"/>
> 
> Do we really want to expose these? IMO we should rather just default
> showInConsole to true (resp. drop it completely). And the strict bit will only
> unnecessarily cause bug reports by users accidentally enabling it...

Either way I'm fine, I guess the ui-review should address that.

> >+<!ENTITY showChrome.label      "Show Internal Errors">
> 
> This sounds like a toggle but isn't (except if you close and reopen the
> console).

This needs to be fixed. Currently, and in my patch it's the same, chrome errors are skipped when showInConsole is false. This creates a problem in that the count of the error console is wrong. The console service will have expired the first few messages if there are chrome errors before the console does. Will fix.


I'll address everything else, and upload a patch for ui-review and code review.

Comment 48

[Simon Bünzli]

(In reply to comment #47)
> What's more, the addons-window is nearly the exact same thing, only they
> rebuild the view with a template

IMO the main difference is that in the Add-ons window the views are meant to be disjunct (the same information isn't available in two different views). In the Console you rather apply a filter to what is always the same view. Things to consider: What should happen once bug 456260 is fixed? What about bug 275265?

Comment 49

[Nochum Sossonko [:Natch]]

(In reply to comment #48)
> consider: What should happen once bug 456260 is fixed? What about bug 275265?

Whatever happens in the first bug will effect the rest of the ui as well as the console, keeping it sync with whatever the latest design is. The latter bug actually makes a lot more sense in this context now, instead of having a whole bunch of buttons or checkboxes determining what shows up in the console, we can have these four main tabs, and then general buttons for further filtering. Overall, I'm not 100% sure of this approach, I think it's cleaner and more inline with the rest of the app, but that's just me :)

Comment 50

[Nochum Sossonko [:Natch]]

Attachment: part 2: give it the new look ver. 2

Leaving this here for ui-review. Nits addressed (btw this is what I needed the QI for).

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #50)
> Leaving this here for ui-review. Nits addressed (btw this is what I needed the
> QI for).

Who should do this? Can you ask someone directly?

Comment 52

[Nochum Sossonko [:Natch]]

(In reply to comment #51)

Well, I'm not really sure who should do this, I figured ui-wanted would attract the correct people, if you know of anyone that should do it, feel free to suggest.
I'm not really in any rush here, I have two bugs that need to land before this one, so I don't mind if it takes a little while (esp. since the error console is hardly ever touched by anyone so maintaining the patch is fairly trivial).

Comment 53

[Nochum Sossonko [:Natch]]

Alex, any chance of getting a ui-review here? No rush really, this is post-3.5 work, just wanted to get this on your "radar".

Comment 54

[Alex Faaborg [:faaborg] (Firefox UX)]

I'll be going through my uiwanted and ui-review queue once we freeze, sorry about the ongoing delays.

Comment 55

[Alex Faaborg [:faaborg] (Firefox UX)]

comment #9
>What's the "mozapp look"?  This looks kinda like the Firefox 2 dlmgr, I guess.

For the ui-review can you explain the rationale behind the interface changes?  Not that I necessarily disagree with them compared to the current UI, just trying to get some more context.

Comment 56

[Nochum Sossonko [:Natch]]

(In reply to comment #55)

For the top part (the tabs) I copies what Firefox uses in other "app windows" (e.g. addons window, options dialog, page info dialog, etc.) to use the blue on white tabs with text underneath the icons. The rest of the change in look comes from the previous bug 305206 where the error console is being changed to use a richlistbox.

The bottom part is the real question, this was just my idea, the button at the far-right is similar to the "clear list" button on the download manager, the checkboxes are next to it as it seemed like the only logical place to put them. Most users won't be using the error console, but for the power users that do, I figured making other options more accessible would be a plus.

Comment 57

[Alex Faaborg [:faaborg] (Firefox UX)]

>I figured making other options more accessible would be a plus.

Yeah, having this increased control for people who use the error console sounds good, we just need to make sure we are organizing the interface in the most logical way (sorry about getting here late and responding at the stage of the ui-review instead of uiwanted).

I'm going to give this some thought and possibly propose an alternative layout.

Comment 58

[Alex Faaborg [:faaborg] (Firefox UX)]

Quick question: are there any performance considerations that go into deciding if internal errors and strict warnings are checked by default?

Comment 59

[Nochum Sossonko [:Natch]]

Internal errors no, strict warnings yes. The internal errors are still reported to the console service whether or not the pref is set, the error console js filters them out, whereas strict warnings are not reported at all unless the pref is set.

Comment 60

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 374274 [details]
screenshot

I'm going to propose an alternate layout for the window to separate out message source and message type and streamline the appearance a little.

Comment 61

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Possible console window minor refresh

>uiwanted

Here is a quick mockup of the same functionality in a different organization.  This interface also fits a little better with other refreshed windows like the Library, and allows us to drop glass behind the Javascript field later on for Vista/7.

What do people think?

Comment 62

[Dão Gottwald [:dao]]

I'm not sure it's quite obvious what "Firefox and Content" means, and I'm pretty sure that UI is irrelevant to the majority of the target audience.

"All Messages" vs. "Messages" seems quirky.

It looks like you couldn't show warnings and errors (but not messages).

I wonder how often people switch between warnings, errors and messages, and whether the dropdown would slow them down.

Comment 63

[Philip Chee]

> I'm not sure it's quite obvious what "Firefox and Content" means, and I'm
> pretty sure that UI is irrelevant to the majority of the target audience.

The audience (at least for the Console² extension) appears to be web developers, extension developers, people hacking Firefox/SeaMonkey/Thunderbird/etc, and amo-editors using it to diagnose other extensions they are reviewing. "Chrome" holds no terrors for these people so talking down to the audience by using "Firefox" would be counter productive. The error console lives in toolkit so hard-coding the application name might be slightly problematic (yes I know you can use &brandShortName;).

> "All Messages" vs. "Messages" seems quirky.

We have an optional combo button called "Type" with a drop down choice of Type: {All|Errors|Warnings|Messages} but our primary UI allows the user to select each type individually.

> I wonder how often people switch between warnings, errors and messages, and
> whether the dropdown would slow them down.

The dropdown definitely slows me down so although it's there if our users want it, I personally don't use it and it's hidden in our default UI.

Comment 64

[Robert Kaiser]

(In reply to comment #61)
> What do people think?

I really like the general approach, it's for sure better than any other suggestion I've seen here so far. As this is a toolkit window though, I think using the Firefox logo can become difficult, as toolkit is app-agnostic. Also, when XULRunner applications are run with |firefox -app| this might lead to unexpected glitches. I think anyone able to understand the actual messages in the error console can understand more abstract references, see comments before. "Chrome" might actually be clearer about what it means in any case (whatever icon we use there, if any).

And I agree on "All Messages" vs. "Messages" sounding strange, but I have no better suggestion right now - "All" as previously is also somewhat suboptimal...

Comment 65

[Dão Gottwald [:dao]]

I'm not sure that Chrome vs. Firefox the right question to ask. Web developers are the majority I was referring to, and they don't need chrome messages, so maybe this should stay a hidden pref.

Regarding the message types dropdown.... If Errors/Warnings/Messages stayed toolbar buttons but became non-exclusive (i.e. checkbox rather than radio type buttons), we could just drop All.

Comment 66

[Dão Gottwald [:dao]]

> I'm not sure that Chrome vs. Firefox

... is the right question to ask.

Comment 67

[Philip Chee]

> If Errors/Warnings/Messages stayed toolbar buttons but became non-exclusive
Yeah, that's the way zeniko did it in Console². We haven't had any complaints at all.

Comment 68

[Robert Kaiser]

And I would be so happy if I could turn off all those bogus content messages and only see the chrome messages, which are what I really care about. ;-)

Comment 69

[Alex Faaborg [:faaborg] (Firefox UX)]

>I'm not sure it's quite obvious what "Firefox and Content" means

Perhaps Firefox and Web?  While this UI is clearly for only advanced users, I'm not sure if Web developers are universally familiar with the Chrome/Content jargon that we regularly use.

>Web developers
>are the majority I was referring to, and they don't need chrome messages, so
>maybe this should stay a hidden pref.

If it only helps Firefox developers than totally agree.  Do these messages help extension and jetpack authors?  If so we might want to still include it, but either way I agree that Web (or content, or whatever) should be the default since Web developers are obviously the majority use case.

>Regarding the message types dropdown.... If Errors/Warnings/Messages stayed
>toolbar buttons but became non-exclusive (i.e. checkbox rather than radio type
>buttons), we could just drop All.

I'm not really sure which behavior is optimal, are people more likely to exclude just 1 out of the 3, or want to display only 1 out of the 3?

>I think
>using the Firefox logo can become difficult, as toolkit is app-agnostic.

Right, can we override the generic name and icon in Firefox builds?

Comment 70

[Nochum Sossonko [:Natch]]

Actually, I think the mode distinctions should change a bit:

Currently we have 3 buttons, Errors Warnings and Messages. The Errors tab lists all "critical" code mistakes, which can be a javascript error (most common) or an xml parsing error. Warnings is far more broad, it lists all non-critical (i.e. code execution/parsing continues) and can be javascript warnings, css warnings, xul warnings, etc. Messages is a generic tab for random string messages chrome reports to the console with the /occasional/ security message.

Maybe this should be extended/changed so that everything relating to javascript can be seen in one tab, css in another and xml/html (all xml messages, which includes xul) in another. I say "extended" because maybe we can use a drop-down under the proposed javascript tab (f.e. same would apply to the others) that would have menu options to filter accordingly (and the drop down can be a check mark menu so that options are non-exclusive).

My reasoning is simple: why would an end user sort based on the message warning level? In my experience, I'm either debugging javascript, css or xml and looking for possible reasons as to why X isn't working or isn't being displayed correctly.

(In reply to comment #69)
> If it only helps Firefox developers than totally agree.  Do these messages help
> extension and jetpack authors?  If so we might want to still include it, but
> either way I agree that Web (or content, or whatever) should be the default
> since Web developers are obviously the majority use case.

These messages help extension developers immensely. Personally, even with all the wonderful addons in place for this, I'll occassionally want to do a quick test for regressions on a specific nightly or on another Firefox branch I don't have readily available. So I download the version I'm interested in and attempt to debug the issue. The error console is the only built-in tool for that.

> I'm not really sure which behavior is optimal, are people more likely to
> exclude just 1 out of the 3, or want to display only 1 out of the 3?

Either way, I think this should be the way to go, as there are certainly many use cases to sort by two.

Comment 71

[Nochum Sossonko [:Natch]]

Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...

Comment 72

[Brian Grinstead [:bgrins]]

The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.

Comment 73

[Zack Weinberg (:zwol)]

I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)

Comment 74

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The error console is gone, so I don't think there's anything to do here.

Comment 75

[Eyal Rozenberg]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #74)
Is that true in Thunderbird as well? Also, what about Seamonkey?